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 #2996
base: main
Are you sure you want to change the base?
feat: support ESLint v9 #2996
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2996 +/- ##
===========================================
+ Coverage 85.67% 95.72% +10.04%
===========================================
Files 78 79 +1
Lines 3275 3301 +26
Branches 1150 1140 -10
===========================================
+ Hits 2806 3160 +354
+ Misses 469 141 -328 ☔ View full report in Codecov by Sentry. |
7c59670
to
543e378
Compare
@ljharb I'm probably going to knock off soon - would you mind having a look over both the changes and CI, and leave me some pointers for the coming days? In addition to just the standard "how do you feel about this code", I'm not sure why some of the builds are failing with "Class constructor RuleTester cannot be invoked without 'new'" as I'm 99% sure my JS syntax is correct so I'd say the compiler is doing it wrong but that seems unlikely...? That could be related to why I found I couldn't use Also I've not yet looked into the But hey we've got about |
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. |
a02c7ea
to
7d42925
Compare
7d42925
to
f3318fb
Compare
5429dab
to
f1e7cd2
Compare
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.
Can you help me understand what withoutAutofixOutput
is for?
const scopeBody = getScopeBody(getScope(context, node)); | ||
log('got scope:', scopeBody); |
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 scope is for the program, i think, so it should be gotten without the node, or with the entire program's node?
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.
Sweet, I'll try passing in the program node and see what happens
@@ -32,7 +32,7 @@ ruleTester.run('no-cycle', rule, { | |||
test({ code: 'var foo = require("./")' }), | |||
test({ code: 'var foo = require("@scope/foo")' }), | |||
test({ code: 'var bar = require("./bar/index")' }), | |||
test({ code: 'var bar = require("./bar")' }), | |||
...usingFlatConfig ? [] : [test({ code: 'var bar = require("./bar")' })], |
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.
why can't this one work in 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.
As it's considered a duplicate of the next line and v9 ruletester doesn't allow duplicate tests
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's not, though - it has a different filename. v9 doesn't consider that to be different? sounds like a bug in 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.
Yup, feel free to raise an issue over at eslint
That was a helper which already existed in one of the tests, so I decided to pull it out as a utility for the others for consistency and then it can be cherry picked ahead of v9. As for why it's required, v9 rule tester considers it an error for the output to match the code - using this utility means (once cherry picked) in this PR we can make one change to the utility (to make output null) rather than have to change the tests for every single rule. Also while I don't mind input, just confirming I'm still chipping away at the general list of failures - I plan to come back later to address the finer details like adding comments :) If someone has the time to look at the failures especially during my night, that's be appreciated - it looks like there's some around regression tests invoking eslint programmatically that I've not been able to figure out how to change while ensuring they're still testing what they're meant to be testing. Even if someone can figure out the general solution, Im happy to handle applying it to all the tests of that type |
Totally, while it's still a draft I'm indeed just chipping away at the review :-) your effort is appreciated! |
@@ -0,0 +1,62 @@ | |||
function getFilename(context) { | |||
if ('filename' in context) { |
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.
To make it easier to refactor in the future, it would be nice to document which branch is for ESLint 9, and which is for the 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.
I think it's pretty obvious given this is the preferred path, and besides Jordan doesn't drop versions so this'll never get refactored
Resolves #2948