[New] support eslint 9 #2996
[New] support eslint 9 #2996
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2996 +/- ##
==========================================
- Coverage 95.20% 95.07% -0.14%
==========================================
Files 82 82
Lines 3565 3571 +6
Branches 1245 1251 +6
==========================================
+ Hits 3394 3395 +1
- Misses 171 176 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ 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
0798272
to
710930e
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Here's that update for the cache key we talked about: #3072 if you want to review. |
This reduces the diff in import-js#2996.
Discovered this issue in import-js#2996 (comment) This change improves the logic for generating the cache key used for both the exportMap cache and resolver cache when using flat config. Prior to this change, the cache key was a combination of the `parserPath`, a hash of the `parserOptions`, a hash of the plugin settings, and the path of the file. When using flat config, `parserPath` isn't provided. So, there's the possibility of incorrect cache objects being used if someone ran with this plugin using different parsers within the same lint execution, if parserOptions and settings are the same. The equivalent cacheKey construction when using flat config is to use `languageOptions` as a component of the key, rather than `parserPath` and `parserOptions`. One caveat is that the `parser` property of `languageOptions` is an object that oftentimes has the same two functions (`parse` and `parseForESLint`). This won't be reliably distinct when using the base JSON.stringify function to detect changes. So, this implementation uses a replace function along with `JSON.stringify` to stringify function properties along with other property types. To ensure that this will work properly with v9, I also tested this against import-js#2996 with v9 installed. Not only does it pass all tests in that branch, but it also removes the need to add this exception: import-js#2996 (comment)
G-Rath
force-pushed
the
support-eslint-v9
branch
from
f3e8eda
to
d1f4b67
Compare
|
@ljharb the codecov check seems to have wandered off entirely now, but I can promise you those lines are now covered so this should be all set |
G-Rath
force-pushed
the
support-eslint-v9
branch
from
e5a76f0
to
3b93140
Compare
G-Rath
force-pushed
the
support-eslint-v9
branch
2 times, most recently
from
925a6a8
to
a5020a6
Compare
|
It looks like removing those two tests caused the coverage on those two lines to fall off again. |
|
It looks like that condition was never covered, so expanding it without adding a new test, is flagging the lack of coverage. If you add a valid test to each rule that passes in |
|
ahh, gotcha. i'll fix them on main and rebase this afterwards. |
ljharb
force-pushed
the
support-eslint-v9
branch
from
a5020a6
to
5a51b9a
Compare
|
@michaelfaith and @G-Rath (and @soryy708), please do file an expense against https://opencollective.com/eslint-plugin-import for what you think is reasonable for all of your contributions in the last year. |
Resolves #2948