Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: run against upcoming next major ESLint version #1534

Merged
merged 16 commits into from
Mar 31, 2024
Merged

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Mar 23, 2024

Hopefully this'll just work... it did not, but now it does!

@G-Rath
Copy link
Collaborator Author

G-Rath commented Mar 29, 2024

It seems like ESLint v9 somehow has different globals resulting in instanceof RegExp no longer working like in jestjs/jest#2549 🤔

@G-Rath
Copy link
Collaborator Author

G-Rath commented Mar 29, 2024

Seems that unbound-method doesn't like being run when eslint-plugin v6 is present as I think it ends up getting a bad version of @typescript-eslint/utils?

I've confirmed this is only a development problem - everything works fine on a standard project that's using eslint-plugin-jest and @typescript-eslint v7 side by side; I'm not sure if we can easily address this in our CI though without just skipping the entire v6 test for next :/

@G-Rath G-Rath force-pushed the run-ci-against-9 branch 3 times, most recently from 5c07150 to 6b74595 Compare March 29, 2024 22:20
@G-Rath G-Rath requested a review from SimenB March 29, 2024 22:38
@G-Rath G-Rath marked this pull request as ready for review March 29, 2024 22:38
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

exciting!

package.json Outdated Show resolved Hide resolved
jest.config.ts Outdated Show resolved Hide resolved
jest.config.ts Outdated
coveragePathIgnorePatterns: ['/node_modules/'],
},
],
} satisfies Config.InitialOptions;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} satisfies Config.InitialOptions;
} satisfies Config;

why satisfies instead of assigning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because assigning means it is Config - so all properties marked as required are such, and optional ones are that outside of the assignment; so TypeScript won't let us do things on those properties later down without existence checks - sadly it's not perfect: we've got to define properties we want to use since the type information is not retained after the assignment, which is why we've got coveragePathIgnorePatterns explicitly defined with its default values.

Having said that, I've not checked out Config.InitialOptions so that might change things

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok so jest.Config is just a re-export of Config.InitialOptions - so yeah, we could do either or; I think using satisfies is slightly nicer though.

To make it a bit clearer:

image

that's what happens if you use assignment, and it's because to TypeScript config is type Config rather than the actual object we created.

@@ -154,6 +155,20 @@ describe('the rule', () => {
expect(() => {
const linter = new TSESLint.Linter();

/* istanbul ignore if */
if (usingFlatConfig()) {
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because we're effectively running ESLint here per usual - so when using ESLint v9 we have to use flatconfig and when using pre-v9 we have to use legacy config.

While linter.verify supports being passed both types of configs, since we have call defineRule in pre-v9 (which errors in v9) this felt the cleanest way to address it (though either way we'll always need usingFlatConfig.

src/rules/__tests__/test-utils.ts Outdated Show resolved Hide resolved

export class FlatCompatRuleTester extends TSESLint.RuleTester {
public constructor(testerConfig?: TSESLint.RuleTesterConfig) {
super(FlatCompatRuleTester._flatCompat(testerConfig));
Copy link
Member

Choose a reason for hiding this comment

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

is it ok to use this private thing?

Copy link
Collaborator Author

@G-Rath G-Rath Mar 29, 2024

Choose a reason for hiding this comment

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

it's our private thing so yes :)

(its static since this is in the constructor and it felt a little nicer than having it as an external function)

src/rules/__tests__/test-utils.ts Outdated Show resolved Hide resolved
src/rules/__tests__/test-utils.ts Outdated Show resolved Hide resolved
@G-Rath G-Rath requested a review from SimenB March 29, 2024 23:27
@G-Rath G-Rath merged commit 59d642c into next Mar 31, 2024
35 checks passed
@G-Rath G-Rath deleted the run-ci-against-9 branch March 31, 2024 18:32
Copy link

github-actions bot commented Apr 6, 2024

🎉 This PR is included in version 28.0.0-next.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link

github-actions bot commented Apr 6, 2024

🎉 This PR is included in version 28.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants