feat: support ESLint v9 #2996
feat: support ESLint v9 #2996
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2996 +/- ##
===========================================
+ Coverage 82.18% 95.37% +13.19%
===========================================
Files 93 82 -11
Lines 4136 3567 -569
Branches 1391 1257 -134
===========================================
+ Hits 3399 3402 +3
+ Misses 737 165 -572 ☔ View full report in Codecov by Sentry. |
G-Rath
force-pushed
the
support-eslint-v9
branch
2 times, most recently
from
7c59670
to
543e378
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
I've just gotten back from travel, so I'll try to take a look at it in the coming days. One likely obstacle is that all the tests assume eslintrc, but eslint 9 requires an env var to support it, otherwise it defaults to flat config. The initial support needs to test eslintrc, and it would be fine to add flat config tests in a follow-on if needed. It's likely that the way eslint < 9's RuleTester works is subtly different than in eslint 9, so we may need an abstraction to handle that. |
|
Yup part of this includes switching to a custom rule tester that converts the tests from eslintrc to flat config if they're running on v9, which is what we've been using in |
|
That's great, but testing eslintrc on eslint 9 is also very important. It'd also be great if there was a commit or two I could land separately, that worked on eslint < 9, so that only the 9-specific parts remained in this PR after that was landed and rebased :-) |
|
If you're meaning the context helper stuff, yeah I'm happy to pull them out I just figured you'd have asked them to be tested against ESLint v9 to prove they actually worked :-) |
|
You figured right :-) but i can just cherry-pick the commits out of this PR once things are working. |
|
Right well they're already good for cherry-picking - you want to pick from e31fe5c...543e378 (sans f0853cb once #2997 is landed), and away you go. I think you can have the eslintrc-on-v9 support by just running the whole test suite twice with the env enabled and an extra test in the custom rule tester - I'll push that up shortly |
|
@ljharb |
|
well that sucks, we’ll need to file an eslint issue about that gap. |
|
I'll leave that to you as I suspect you'll have more pull :-) |
|
Filed eslint/eslint#18292. |
G-Rath
force-pushed
the
support-eslint-v9
branch
2 times, most recently
from
7d42925
to
f3318fb
Compare
G-Rath
force-pushed
the
support-eslint-v9
branch
from
5429dab
to
f1e7cd2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Totally, while it's still a draft I'm indeed just chipping away at the review :-) your effort is appreciated! |
G-Rath
force-pushed
the
support-eslint-v9
branch
from
85a0add
to
23260f0
Compare
|
(codecov seems to be getting confused - sometimes its happy, sometimes its not 🤷) |
G-Rath
force-pushed
the
support-eslint-v9
branch
4 times, most recently
from
cbd057a
to
6073c7f
Compare
|
Getting greenly exciting! |
|
All passed！ |
|
A quick note to @ljharb on ordering. These two changes from the
And this PR has also absorbed this change from the core package, which hasn't merged yet: #3062 |
G-Rath
force-pushed
the
support-eslint-v9
branch
from
6073c7f
to
5de2845
Compare
G-Rath
force-pushed
the
support-eslint-v9
branch
from
5de2845
to
587b03e
Compare
|
@ljharb in addition to the PRs mentioned by @michaelfaith, I would start with reviewing these and cherry-picking them first when you're happy: |
|
Really appreciate your commitment to this work @G-Rath. You've put some real time into this, and it'll be a big win once it lands. |
| @@ -116,6 +116,11 @@ exports.default = function parse(path, content, context) { | |||
| parserOptions = Object.assign({}, parserOptions); | |||
| parserOptions.ecmaFeatures = Object.assign({}, parserOptions.ecmaFeatures); | |||
|
|
|||
| // fallback to the ecma version in languageOptions when using flat config | |||
| if (context.languageOptions) { | |||
| parserOptions.ecmaVersion = parserOptions.ecmaVersion || context.languageOptions.ecmaVersion; | |||
There was a problem hiding this comment.
I would suggest removing this change in favor of what I added to this PR: #3061 which gets both ecmaVersion and sourceType. Also changes to utils need to be released separate. So it may make more sense to let that other PR go through and absorb that.
There was a problem hiding this comment.
Yeah this'll go away once #3061 is landed - I'm not removing it for now because I need it for CI to be green and Jordan wants to cherry-pick stuff out anyway so there will be at least one "grand rebase" of this pull request before it gets merged 😄
Resolves #2948