Skip to content

Conversation

@Lms24
Copy link
Member

@Lms24 Lms24 commented Aug 6, 2024

Discovered today that TS 3.8 doesn't understand // @ts-expect-error. Since we test on Node 22 also with TS 3.8, we shouldn't use ts-expect-error in test files as TS 3.8 would simply ignore the comment and throw a type error.

Instead, it's fine to use @ts-ignore in these test files.

@Lms24 Lms24 changed the base branch from develop to lms/feat-core-metaTags August 6, 2024 12:27
Base automatically changed from lms/feat-core-metaTags to develop August 6, 2024 12:38
@Lms24 Lms24 force-pushed the lms/chore-lint-tsignore-node-integ-tests branch from 55fcf00 to 0f13518 Compare August 6, 2024 12:38
@Lms24 Lms24 self-assigned this Aug 6, 2024
@Lms24 Lms24 marked this pull request as ready for review August 6, 2024 12:38
@Lms24 Lms24 requested review from a team, andreiborza and mydea and removed request for a team August 6, 2024 12:38

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
// @ts-expect-error - response is defined, types just don't reflect it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait but is this not introducing a ts-expect-error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol thx good catch! I guess I changed it back to expect-error to test the rule and the rule was incorrect so it actually still passed 🤦‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, I adjusted the rule, now it actually throws when you use @ts-expect-error

@Lms24 Lms24 merged commit 69d4823 into develop Aug 6, 2024
@Lms24 Lms24 deleted the lms/chore-lint-tsignore-node-integ-tests branch August 6, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants