feat: support ESLint v9 #2996

G-Rath · 2024-04-07T00:37:09Z

Resolves #2948

codecov · 2024-04-07T00:39:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.74%. Comparing base (a7b4348) to head (9835eb0).

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.
📢 Have feedback on the report? Share it here.

ljharb · 2024-04-08T04:18:26Z

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.

G-Rath · 2024-04-08T04:33:15Z

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 eslint-plugin-jest without issue.

ljharb · 2024-04-08T05:01:32Z

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 :-)

G-Rath · 2024-04-08T05:03:42Z

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 :-)

ljharb · 2024-04-08T05:05:11Z

You figured right :-) but i can just cherry-pick the commits out of this PR once things are working.

G-Rath · 2024-04-08T05:07:47Z

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

G-Rath · 2024-04-08T05:19:33Z

@ljharb RuleTester does not support eslintrc in v9 - the env variable is only used by the CLI

ljharb · 2024-04-08T06:03:31Z

well that sucks, we’ll need to file an eslint issue about that gap.

G-Rath · 2024-04-08T06:06:23Z

I'll leave that to you as I suspect you'll have more pull :-)

ljharb · 2024-04-08T16:17:20Z

Filed eslint/eslint#18292.

tests/src/rule-tester.js

ljharb

Can you help me understand what withoutAutofixOutput is for?

ljharb · 2024-04-12T05:16:20Z

src/rules/newline-after-import.js

+          const scopeBody = getScopeBody(getScope(context, node));
+          log('got scope:', scopeBody);


the scope is for the program, i think, so it should be gotten without the node, or with the entire program's node?

Sweet, I'll try passing in the program node and see what happens

i think this still needs addressing

ljharb · 2024-04-12T05:20:09Z

tests/src/rules/no-cycle.js

@@ -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")' })],


why can't this one work in flat config?

As it's considered a duplicate of the next line and v9 ruletester doesn't allow duplicate tests

it's not, though - it has a different filename. v9 doesn't consider that to be different? sounds like a bug in v9.

Yup, feel free to raise an issue over at eslint

in that case, let's add a code comment or something so we can keep both test cases.

ljharb · 2024-04-12T05:47:22Z

Totally, while it's still a draft I'm indeed just chipping away at the review :-) your effort is appreciated!

src/context.js

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

michaelfaith · 2024-09-04T22:16:41Z

Where are you seeing that error?

When you run the tests for this branch using v9

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

From https://github.com/import-js/eslint-plugin-import/pull/2996/files#diff-9b77313b40bf7cb5c34df649f6938c118c11681ecbc6215d1242d55cd58ae4aaR33

michaelfaith · 2024-09-05T11:03:29Z

Looks like some people are experiencing the same error when trying to use flat config, even outside of the RuleTester context. I noticed you commented on this issue in the eslint repo about the same error a few months ago: eslint/eslint#17953 (comment), and it seemed that according to @mdjermanovic, the context should always have the parser on the context (through languageOptions), but what I'm seeing when logging locally is that languageOptions isn't on the context at all, if the user hasn't defined it in the config. And so, since parserPath isn't defined with flat config, and there's no languageOptions to get the parser, the code in this plugin's parse function bails out, since it can't find a parser to use. So, the question that comes to mind for me, is: is that a bug in eslint and the context should have these options on it always, or does that parse code need to be more resilient to fallback to a default parser if the context doesn't have it?

michaelfaith · 2024-09-05T11:22:54Z

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.

michaelfaith · 2024-09-05T11:31:15Z

@G-Rath I believe this will fix the issue you're seeing. This has been a problem for a while, it just didn't rear it's head until you start using flat config (because parserPath isn't there to fallback on anymore) #3052

* 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

cthompson-avb · 2024-09-05T19:59:06Z

This PR STILL does not update the peerDependencies:

  "peerDependencies": {
    "eslint": "^2 || ^3 || ^4 || ^5 || ^6 || ^7.2.0 || ^8"
  },

this causes npm to install eslint 8.57.0 because v9 is not in the range.

G-Rath · 2024-09-05T20:01:27Z

@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

michaelfaith · 2024-09-05T21:47:30Z

tests/src/rules/namespace.js

@@ -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 } });


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.

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 } ]

we don't need to use the latest version of the globals package tho.

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)

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?

Looks like this is another bug similar to #3052 - if I apply that manually to v2.30.0 and then lint using a plain project I get the same error. 26eba2e seems to fix it, though I'm not 100% sure if there's a gotcha somewhere but we'll see what CI has to say 😄

cthompson-avb · 2024-09-05T22:06:43Z

@cthompson-avb this PR STILL is a work-in-progress - I will get to that, when I get to that

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.

…t config

G-Rath force-pushed the support-eslint-v9 branch 2 times, most recently from 7c59670 to 543e378 Compare April 7, 2024 01:10

This comment was marked as outdated.

Sign in to view

antongolub mentioned this pull request Apr 7, 2024

[Feat]: provide compatibility with eslint v9 jsx-eslint/eslint-plugin-react#3727

Closed

ljharb mentioned this pull request Apr 8, 2024

Bug: Unable to test rules in v9 in both flat config and eslintrc eslint/eslint#18292

Closed

1 task

G-Rath force-pushed the support-eslint-v9 branch 2 times, most recently from 7d42925 to f3318fb Compare April 8, 2024 19:09

csouchet mentioned this pull request Apr 9, 2024

chore(deps-dev): bump eslint from 8.57.0 to 9.0.0 in the lint group process-analytics/process-analytics.dev#1254

Closed

kuoruan reviewed Apr 9, 2024

View reviewed changes

tests/src/rule-tester.js Outdated Show resolved Hide resolved

G-Rath force-pushed the support-eslint-v9 branch from 5429dab to f1e7cd2 Compare April 12, 2024 03:46

ljharb reviewed Apr 12, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

burtek mentioned this pull request Apr 12, 2024

feat(deps)!: update dependency eslint to v9 - autoclosed burtek/eslint-config#11

Closed

1 task

ahocevar mentioned this pull request Apr 16, 2024

Update dependencies for eslint v9 support openlayers/eslint-config-openlayers#107

Open

ljharb mentioned this pull request Apr 18, 2024

Support eslint v9 #2948

Open

wojtekmaj reviewed Apr 19, 2024

View reviewed changes

src/context.js Outdated Show resolved Hide resolved

ricardogobbosouza mentioned this pull request Apr 24, 2024

refactor!: use eslint 9 webpack-contrib/eslint-config-webpack#29

Open

4 tasks

david-luna mentioned this pull request Apr 30, 2024

chore: upgrade eslint elastic/elastic-otel-node#155

Draft

michaelfaith mentioned this pull request Sep 4, 2024

📈 Tracking: Flat Config support eslint/eslint#18093

Open

39 tasks

This comment was marked as resolved.

Sign in to view

G-Rath force-pushed the support-eslint-v9 branch from f1d5b8c to 0ef4e0d Compare September 5, 2024 19:24

michaelfaith reviewed Sep 5, 2024

View reviewed changes

G-Rath added 8 commits September 6, 2024 12:27

feat: support ESLint v9

80c9be8

test: use flat compat rule tester

f060d56

test: try this babel class workaround

8d516aa

test: only require parser values that are strings

71dc2ab

test: don't include output when nothing has been changed

42bc59d

test: skip duplicate examples

52eb1f7

test: set ecmaVersion rather than env

8876362

fix: replace missed usage of context.getScope

26f5e06

G-Rath force-pushed the support-eslint-v9 branch from 0ef4e0d to 26f5e06 Compare September 6, 2024 00:38

G-Rath added 2 commits September 6, 2024 13:27

test: remove some duplicates

934f59e

fix: replace missed usage of context.getAncestors

1a51e0f

G-Rath force-pushed the support-eslint-v9 branch from 0e2ee13 to 1a51e0f Compare September 6, 2024 02:01

G-Rath added 2 commits September 6, 2024 15:09

fix: ensure the ecma version is correct when calling parser using fla…

d64b887

…t config

fix: update declaredScope to use new getScope when possible

1d63e82

G-Rath force-pushed the support-eslint-v9 branch from 9835eb0 to 1d63e82 Compare September 6, 2024 03:09

		const scopeBody = getScopeBody(getScope(context, node));
		log('got scope:', scopeBody);

feat: support ESLint v9 #2996

Are you sure you want to change the base?

feat: support ESLint v9 #2996

Conversation

G-Rath commented Apr 7, 2024

codecov bot commented Apr 7, 2024 • edited Loading

Codecov Report

This comment was marked as outdated.

ljharb commented Apr 8, 2024

G-Rath commented Apr 8, 2024

ljharb commented Apr 8, 2024

G-Rath commented Apr 8, 2024

ljharb commented Apr 8, 2024

G-Rath commented Apr 8, 2024

G-Rath commented Apr 8, 2024

ljharb commented Apr 8, 2024

G-Rath commented Apr 8, 2024

ljharb commented Apr 8, 2024

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

ljharb commented Apr 12, 2024

This comment was marked as resolved.

michaelfaith commented Sep 4, 2024

This comment was marked as resolved.

michaelfaith commented Sep 5, 2024 • edited Loading

michaelfaith commented Sep 5, 2024

michaelfaith commented Sep 5, 2024

cthompson-avb commented Sep 5, 2024

G-Rath commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G-Rath Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cthompson-avb commented Sep 5, 2024

codecov bot commented Apr 7, 2024 •

edited

Loading

michaelfaith commented Sep 5, 2024 •

edited

Loading

G-Rath Sep 5, 2024 •

edited

Loading