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: support ESLint v9 #9002
base: v8
Are you sure you want to change the base?
feat: support ESLint v9 #9002
Conversation
Thanks for the PR, @JoshuaKGoldberg! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
package.json
Outdated
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.
> nx run eslint-plugin:lint
Linting "eslint-plugin"...
Error: You have attempted to use the lint rule deprecation/deprecation which requires the full TypeScript type-checker to be available, but you do not have `parserOptions.project` configured to point at your project tsconfig.json files in the relevant TypeScript file "overrides" block of your project ESLint config `packages/eslint-plugin/.eslintrc.json`
Please see https://nx.dev/guides/eslint for full guidance on how to resolve this issue.
...I haven't dug in to know exactly why this is happening, but my hunch is that it's a dependency version mismatch. Something around gund/eslint-plugin-deprecation#79 / gund/eslint-plugin-deprecation#86. Alternately, it'd be solved on our end by #8988.
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.
It looks like this was previously reported to Nx in nrwl/nx#20731. I tried the remediations there but couldn't find the magic combo to get this to work locally. Filed nrwl/nx#23088 to request more informative errors.
cc @JamesHenry
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.
Awesome! LGTM 🏆
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.
question: do we really want to do the bump to v9 if it completely breaks our lint setup like this?
would we be better served by instead just adding a decent integration test which runs against v9, and leaving our setup on v8 for now?
eslint.config.mjs
Outdated
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.
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.
big ole oof
packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars-eslint.test.ts
Show resolved
Hide resolved
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 filename is wrong if this is a flat config?!
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.
also - this fixture is called "legacy-config"
so should probably use a legacy config?
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.
That's what I was going for with the .eslintrc.js
. As in, using that as the legacy config.
eslintIntegrationTest(__filename, '*.ts', true);
...did I miss something?
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.
I meant - this file is named .eslintrc
but the contents are a flat config.
So things are mixed up - the contents should be a legacy config instead, right?
@@ -16,7 +16,7 @@ exports[`flat-config-types eslint should work successfully 1`] = ` | |||
"line": 38, | |||
"message": "'_otherCases' is defined but never used.", | |||
"messageId": "unusedVar", | |||
"nodeType": null, | |||
"nodeType": "Identifier", |
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 should still be null
based on how the rule works?
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.
Ah, might be a merge thing - merging everything in...
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.
Threading @bradzacher's #9002 (review):
question: do we really want to do the bump to v9 if it completely breaks our lint setup like this?
would we be better served by instead just adding a decent integration test which runs against v9, and leaving our setup on v8 for now?
I think the plugins not supporting ESLint v9 aren't super damaging to us:
deprecation/deprecation
: is something we keep a close eye on anyway, and will be resolved in Rule proposal: Warn when deprecated APIs are used (eslint-plugin-deprecation) #8988 (which IIRC you'd previously mentioned a 👍 on somewhere?)import/
: is mostly stylistic, and imo not exactly the most impactful set of rules we have - expect forno-mutable-exports
, which we generally look out for in review anywayreact
is something we didn't even have enabled for a while anyway, and our playground isn't a high traffic code areareact-hooks
will be updated with React 19
🤷 I don't love that we're losing this bit of our linting, but for me it's less valuable than us more thoroughly testing ESLint v9.
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.
Finished packing local packages. FAIL tests/flat-config-types.test.ts (59.943 s) ... Tests: 3 failed, 7 passed, 10 total
I broke this in the latest update / merge, and am out of coding time for now. 😂
PR Checklist
Overview
Bumps our dependency ranges to include ESLint 9 and its associated packages. I added a few more
resolutions
because some of our lint dependencies still are on older versions of those associated packages. I think we can tackle updating dependencies as a followup.Updates most integration tests to the new flat config format. Adds in a new
legacy-config
integration test so we still have explicit coverage of eslintrc.Otherwise uses
configType: 'eslintrc'
for all instances ofnew Linter
so that we don't have to change them to supporting flat config logic yet. That's actually a non-trivial change. For exampleRuleTester
would need to refactor when it adds rules to aLinter
instance. I think we can tackle refactoring to flat config for internalLinter
instances as a followup. It'll be necessary for ESLint 10.Comments out usage of a few plugins in our own internal lint config pending their support of ESLint 9:
eslint-plugin-deprecation
: ESLint v9 compatibility gund/eslint-plugin-deprecation#78eslint-plugin-react
: ESLint v9 contains breaking API changes jsx-eslint/eslint-plugin-react#3699eslint-plugin-react-hooks
: eslint-plugin-react-hooks & "Flat Config" (ESLint 9) facebook/react#28313