-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2996 +/- ##
==========================================
- Coverage 95.05% 91.74% -3.32%
==========================================
Files 82 82
Lines 3437 3439 +2
Branches 1185 1187 +2
==========================================
- Hits 3267 3155 -112
- Misses 170 284 +114 ☔ View full report in Codecov by Sentry. |
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. |
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
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 this still needs addressing
@@ -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
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.
in that case, let's add a code comment or something so we can keep both test cases.
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! |
This comment was marked as resolved.
This comment was marked as resolved.
This change adds helper functions to `eslint-module-utils` in order to add eslint v9 support to `eslint-plugin-import` in a backwards compatible way. Contributes to import-js#2996
Perhaps I'm missing something, is it in a local version you haven't pushed yet, because I'm seeing a different error in the CI runs: https://github.com/import-js/eslint-plugin-import/actions/runs/10676137395/job/29589199260?pr=2996#step:5:17 |
This comment was marked as resolved.
This comment was marked as resolved.
Looks like some people are experiencing the same error when trying to use flat config, even outside of the |
Actually, I think I might have figured it out why the parser isn't making it through on the context. I should be able to get a fix in later today. |
* use the rc candidate for eslint-plugin-react-hooks * and remove eslint-plugin-import, wait for github.com/import-js/eslint-plugin-import/pull/2996 * add recommended plugins, but may remove them since incompatible plugins with peer deps throw npm install guard
f1d5b8c
to
0ef4e0d
Compare
This PR STILL does not update the
this causes npm to install eslint 8.57.0 because v9 is not in the range. |
@cthompson-avb this PR STILL is a work-in-progress - I will get to that, when I get to that |
This change adds helper functions to `eslint-module-utils` in order to add eslint v9 support to `eslint-plugin-import` in a backwards compatible way. Contributes to import-js#2996
tests/src/rules/namespace.js
Outdated
@@ -1,5 +1,5 @@ | |||
import { test, SYNTAX_CASES, getTSParsers, testVersion, testFilePath, parsers } from '../utils'; | |||
import { RuleTester } from 'eslint'; | |||
import { FlatCompatRuleTester as RuleTester } from '../rule-tester'; | |||
import flatMap from 'array.prototype.flatmap'; | |||
|
|||
const ruleTester = new RuleTester({ env: { es6: true } }); |
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 line is where the env
testing error is coming from in the CI failures. Looks to be the only test that passes in an env
.
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.
correct - I've resolved that locally by replacing it with setting the ecmaVersion
since mapping env
to globals
has turned out to be very "interesting" especially as the latest version of the globals
package requires Node v18+.
Next issue is that ESLint doesn't want to treat the imported files by this test as a module
:
AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: Should have no errors but had 1: [
{
ruleId: 'rule-to-test/namespace',
severity: 1,
message: "Parse errors in imported module './named-exports': sourceType 'module' is not supported when ecmaVersion < 2015. Consider adding `{ ecmaVersion: 2015 }` to the parser options. (undefined:undefined)",
line: 1,
column: 24,
nodeType: 'Literal',
endLine: 1,
endColumn: 41
}
]
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 don't need to use the latest version of the globals
package tho.
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've not looked into it too deeply yet, but I believe the way globals work in v9+ has changed to require strings instead of booleans which means older versions of globals
don't work out of the box (at least that's what the errors I got seemed to indicate) + I expect that we might need to use a very old version of the package for Node v4 support.
Ultimately right now I've gone this route because as @michaelfaith said this seems to be the only test that actually uses env
and looks like it's only doing so for the ecma version rather than any global, plus I can't find any other test using globals - so I'm going with this as the easier option, but I'm not against revisiting properly supporting env
later or in a follow-up PR if you'd like that (assuming it doesn't turn out to be needed anyway for this PR)
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.
Next issue is that ESLint doesn't want to treat the imported files by this test as a
module
:
named-exports.js
would be considered a module. In the rule-tester compat, you could transform the env
syntax to ecmaVersion
on parser options and map es6 to 2015?
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. I just don't want you to get too excited when everything's working and forget :-D . I appreciate the work you're doing. |
0ef4e0d
to
26f5e06
Compare
0e2ee13
to
1a51e0f
Compare
9835eb0
to
1d63e82
Compare
Resolves #2948