-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat: Add eslint v9 support #3743
feat: Add eslint v9 support #3743
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can’t support anything we’re not testing, and this doesn’t add v9 to tests.
I know, that's why this PR is still a draft. I just started working on eslint v9 adjustments based on this article which explains what should be changed in eslint v9. If there is already an ongoing work or the support of the eslint v9 is not intended to be added in the near future, I can close this PR. |
Support is definitely intended :-) #3726 is another attempt. |
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
I fixed most of failing tests and adjusted them to eslint v9 style. There are 5 remaining rules which tests still don't pass. I can get back to fixing them later. I wanted to ask you, if you have an idea how to handle linting in this repo? I saw that I wonder if creating yarn/npm workspaces with different eslint versions would be a valid solution for you and if it will solve the problem. |
parserOptions: { | ||
languageOptions: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like it will break on older eslint versions (and eslintrc config), which we still need to retain support for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was made only in tests and is necessary after upgrading to eslint v9. I don't know if we should keep separate tests for older eslint versions. Maybe we should somehow duplicate all tests to test rules separately with eslint v9 and with older versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative approach is to use eslint v8 as a dev dependency and test rules with eslint v8 as it was done before, and add v9 to peerDependencies but don't test rules with the latest eslint. I saw this approach in the eslint-plugin-perfectionist repo, where eslint v9 support was added but tests are still written in v8. This plugin dropped support for eslint v7 (and versions before), though, as the new eslint API that we have to use in v9 was introduced in v8 and the old API was removed in v9 (see this article).
Don't worry about the linting yet; the import plugin has to come first for that. However, we already have to retain compatibility for every eslint version we currently support, so we can't switch things over to just work on v9, or just on flat config. If we used workspaces, we'd only use npm, but I don't think that's necessary - the way we support multiple eslint versions now is by manually altering the deps in CI, and we'd do that here as well. |
I see, thanks for clarification. I still wonder what will be the best way to ensure that tests work in all eslint versions. v9 moved |
We'll probably need a helper function that detects the eslint version and adjusts appropriately. The smallest change for now would be to leave the tests largely as-is, and have the helper change them to the v9 format when applicable - later, we'd probably want to invert that, but that can wait. |
Upgrade to eslint v9 flat config. Custom eslint rules have been tested and work in the new config file. Use [globals](https://www.npmjs.com/package/globals) to include [globals env](https://eslint.org/docs/latest/use/configure/language-options#predefined-global-variables) eslint-plugin-react since it does not support eslint 9 yet Ref: jsx-eslint/eslint-plugin-react#3743 no qa required
I think that #3759 obsoletes this, but if not please reopen it and rebase it. |
Description
This PR adds eslint v9 support. It also adjusts respective tests to use v9-style config.
TODO
Fixing tests
I got most of tests working by simply changing the config used in tests or removing duplicate test cases. Below is the list of rules which tests still are failing and fixing them will require a bit more effort. I will continue fixing tests in my free time.
Fixing eslint config
After upgrading
eslint
to v9, the.eslintrc
config used in theeslint-plugin-react
repo is no longer valid. We should either migrate from.eslintrc
config toeslint.config.js
, or use old version ofeslint
just for linting in this repository.Because
.eslintrc
config is no longer valid, CI shows that all tests are failing. I ran test files directly with the following command while fixing test issues: