Fix ESLint v10 RuleContext API removal (follow-up to #3972) - #3979
Fix ESLint v10 RuleContext API removal (follow-up to #3972)#3979ledsun wants to merge 2 commits into
Conversation
ljharb
left a comment
There was a problem hiding this comment.
we'll need regression tests - it might be as simple as adding eslint 10 to the GHA matrix.
f987ffa to
a7b967b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3979 +/- ##
==========================================
- Coverage 97.71% 97.56% -0.15%
==========================================
Files 137 134 -3
Lines 10188 10177 -11
Branches 3797 3797
==========================================
- Hits 9955 9929 -26
- Misses 233 248 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
6a95ab9 to
13c7805
Compare
|
@ledsun do you have time to address the failing checks. I'm looking at updating to eslint 10 and we're using the eslint-plugin-react |
I took a look at the logs for the first few failures. I'm wondering @ljharb , if it's time to prune the test matrix. The deprecation warnings for many dependencies are pretty dire ["unsupported" is the least dramatic]. It's very hard to tell whether there's actually a problem with this pull request or whether the matrix is untestable, as-is. |
|
I don't think it will get everything passing, but I believe this patch will get us closer, if it is an acceptable approach. EDIT: To explain what's going on: New TypeScript parsers need a newer TypeScript. The tests were giving them an old TypeScript, so they crashed. The older, deprecated parser has the opposite problem: it cannot work with the newer TypeScript. So the patch does this: When testing a modern TypeScript parser, install TypeScript 5 so that parser can run. In those same tests, don’t also test the deprecated parser, because it cannot run with TypeScript 5. There are still other problems, so I think there will still be tests failures to sort through, but this should greatly reduce the noise. But I don't know if this approach is compatible with the compatibility goals. (I sure hope so, though.) diff --git a/.github/workflows/eslint-8-.yml b/.github/workflows/eslint-8-.yml
index 42168ae7..55b049a9 100644
--- a/.github/workflows/eslint-8-.yml
+++ b/.github/workflows/eslint-8-.yml
@@ -100,7 +100,7 @@ jobs:
with:
node-version: ${{ matrix.node-version }}
after_install: |
- npm install --no-save "eslint@${{ matrix.eslint }}" "@typescript-eslint/parser@${{ matrix.typescript-eslint == 8 && 8.17 || matrix.typescript-eslint }}" "babel-eslint@${{ matrix.babel-eslint }}"
+ npm install --no-save "eslint@${{ matrix.eslint }}" "@typescript-eslint/parser@${{ matrix.typescript-eslint == 8 && 8.17 || matrix.typescript-eslint }}" "babel-eslint@${{ matrix.babel-eslint }}" ${{ matrix.typescript-eslint >= 6 && '"typescript@5"' || '' }}
env:
NPM_CONFIG_LEGACY_PEER_DEPS: "${{ matrix.typescript-eslint >= 6 && 'false' || 'true' }}"
- run: npx ls-engines
diff --git a/.github/workflows/eslint-9+.yml b/.github/workflows/eslint-9+.yml
index 91a8ddc4..6c889ea9 100644
--- a/.github/workflows/eslint-9+.yml
+++ b/.github/workflows/eslint-9+.yml
@@ -50,7 +50,7 @@ jobs:
with:
node-version: ${{ matrix.node-version }}
after_install: |
- npm install --no-save "eslint@${{ matrix.eslint }}" "@typescript-eslint/parser@${{ matrix.eslint == 10 && 'canary' || (matrix.typescript-eslint == 8 && 8.17 || matrix.typescript-eslint) }}" "babel-eslint@${{ matrix.babel-eslint }}" ${{ matrix.eslint == 10 && '"typescript@5"' || '' }}
+ npm install --no-save "eslint@${{ matrix.eslint }}" "@typescript-eslint/parser@${{ matrix.eslint == 10 && 'canary' || (matrix.typescript-eslint == 8 && 8.17 || matrix.typescript-eslint) }}" "babel-eslint@${{ matrix.babel-eslint }}" "typescript@5"
skip-ls-check: true
- run: npx ls-engines
- run: npm run unit-test
diff --git a/tests/helpers/parsers.js b/tests/helpers/parsers.js
index f0a1e679..a9fe5ecb 100644
--- a/tests/helpers/parsers.js
+++ b/tests/helpers/parsers.js
@@ -150,8 +150,11 @@ const parsers = {
|| features.has('jsx namespace')
|| features.has('bind operator')
|| features.has('do expressions');
- // typescript-eslint-parser (deprecated) cannot parse a TS 5 tsconfig, used by the eslint 10 matrix.
- const tsOld = !skipTS && !features.has('no-ts-old') && !semver.satisfies(version, '>= 10');
+ // typescript-eslint-parser (deprecated) cannot parse the TS 5 tsconfig used with modern TS parsers.
+ const tsOld = !skipTS
+ && !features.has('no-ts-old')
+ && !semver.satisfies(version, '>= 10')
+ && !semver.satisfies(tsParserVersion, '>= 6');
const tsNew = !skipTS && !features.has('no-ts-new');
return [].concat( |
|
@ljharb What is the right approach to take with
One thing I wanted to do which does not seem possible to reliably do is "Check if (Apologies if this is covered above. I'm having trouble getting GitHub to load the entire conversation.) |
|
Here's a branch that adds three commits on top of the two that are in this PR. It gets us most of the way there. Just 18 remaining failed tests, 10 of which are the |
haimuhaimu
left a comment
There was a problem hiding this comment.
Reviewed the new ESLint 10 matrix against the package published Node.js engine range and the generated Actions cells.
Finding (.github/workflows/eslint-9+.yml, matrix exclusions around lines 37-44): the >=18 matrix also generates Node 21 and 23, but eslint@10.8.0 declares engines.node as ^20.19.0 || ^22.13.0 || >=24. The current Actions run contains ESLint 10 jobs for both unsupported odd-numbered releases, so engine-related failures in those cells cannot validate this plugin ESLint 10 support. Could we exclude Node 21 and 23 as well and update the comment from the broader “20+” wording, or derive the supported cells from the ESLint engine range?
@ledsun : how does this patch look to you? getting us down to |
@ljharb : it does seem like the matrix is extremely wide-ranging, possibly into version permutations nobody would be using in practice today. I believe we are all in support of maximalist compatibility: you never know what weird edge cases result in strange version pin combinations out in the wild. That said, eventually pruning long-unsupported versions is also thing. |
|
I have prepared a complete, 100% green patch branch built on top of this PR (#3979) and subsequent community contributions, fully resolving all remaining unit test failures under ESLint 10. Patched Branch: plthomasva/eslint-plugin-react@update-deprecated-calls-v8 History & Summary of Integrated ChangesThis branch consolidates and builds upon the work of several contributors across the PR history:
Verification ResultsAll 11,923 unit tests pass 100% locally on ESLint 10:
AcknowledgmentThese additional changes and verification were developed with the assistance of Antigravity and the Gemini 3.6 Flash model. |
|
@plthomasva Please note that I am not a maintainer, so this is not advice implying that “if you recreate the pull request, it can be merged.” |
|
I'm a little worried that this doesn't actually make as much progress as I thought based on the simulated matrix run in PR#1 of my fork. :( |
|
Any update on this? |
|
|
Everyone, thank you for your patience! I'm brand new to this repository, so it took me a while to massage the test matrix. #4022 has a clean board and is just awaiting maintainer review and any changes needed. It is a breaking change, but it's the easiest kind of breaking change. Anyone who still wants to use the (generally abandoned by now) oldest ESLint versions will have to pin to v7.x here. Similarly, anyone who wants to use Node versions less than 16 will have to pin, becuse ESLint no longer supports them. I don't really see these as particularly breaking: You aren't using the ESLint React plugin without ESLint! See you over in comments on my follow-on PR. |
|
Great work and thank you for your efforts! @plthomasva This is getting increasingly urgent as ESLint 9.x goes EOL on August 6th, so any comment from maintainers would be nice.. |
|
Moved to Oxlint while waiting. |
Did so too but did you manage to replicate all of the rules with oxlint? |
This PR follows up on the feedback in the comments of #3972 and incorporates the requested changes.
I think this PR will fix #3977.