ESLint v10 support#3230
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3230 +/- ##
==========================================
- Coverage 95.50% 93.87% -1.64%
==========================================
Files 83 83
Lines 3693 3755 +62
Branches 1336 1355 +19
==========================================
- Hits 3527 3525 -2
- Misses 166 230 +64 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@rasmi Do you want to disclose your usage of LLMs to generate this PR description and/or PR, which @ljharb objected to in another repo jsx-eslint/eslint-plugin-jsx-a11y#1077 ? |
|
Was not aware, thanks @StephanTLavavej! It is a bit disappointing, as I spent a few hours of my day working on this, even with LLM tools. I will close this PR regardless out of respect for the maintainer (I just want to make sure the tests pass for my own edification). |
rasmi
force-pushed
the
eslint-update
branch
3 times, most recently
from
efa4b8b to
069a70f
Compare
|
Just curious (and with all respect), what is the real issue of LLM-generated code? What matters is the code, right? If the PR is too big, or too hard to review, we could just improve that? |
|
@StephanTLavavej please do not take it upon yourself to act as if you're a maintainer. Each project can have different policies regardless of who's maintaining them. Please do not attempt to police projects you aren't even a significant contributor to. @rasmi if you're using an LLM to automate applying changes you're writing, I'm fine with that - however if you're just prompting one to "add eslint v10 support", i have my own LLM subscription for that kind of thing :-) @dirkluijk no, "what matters" has never been, and never will be, just "the code". Any code contribution represents a legal assignment of rights, as well as an eternal maintenance burden on that code, and as such, the human contributor is an important factor. |
|
@ljharb -- point well-taken! I did go through a more rigorous process here -- it really did take me hours of focused work, thinking about implementation approaches, iterating, fixing tests, etc. It is not my intention to submit slop, I wanted to submit a PR that I hoped would meet the standard of the library, at least as a first pass. I lack decision-making insight/context on things like "Is it okay to just use glob, if not let's do a custom file search" and "exactly how should node vs. eslint-parser vs. core library version compatibility be handled" -- but this is what the review process is for! I also understand that big changes like this may be simpler for the maintainer to just take on directly, as you have all the proper context and expertise on all the nuances of the library and ecosystem, to say the least. Apologies again, I didn't mean to burden you at all, I know it is hard to maintain core libraries like this. Please do with this what you wish! |
| beforeEach(function () { | ||
| if (semver.satisfies(eslintPkg.version, '< 6')) { | ||
| if (semver.satisfies(eslintPkg.version, '< 6') || semver.satisfies(eslintPkg.version, '>= 10')) { | ||
| // TODO: re-enable when eslint-plugin-json supports v10 (https://github.com/azeemba/eslint-plugin-json/issues/97) |
There was a problem hiding this comment.
Maybe consider switching to https://www.npmjs.com/package/eslint-plugin-jsonc
There was a problem hiding this comment.
good idea, last commit in https://github.com/azeemba/eslint-plugin-json was 17 months ago
There was a problem hiding this comment.
I'm fine with that switch, in its own commit.
There was a problem hiding this comment.
(Failing on older versions, will fix this eve) fixed!
|
What is the current status of this PR? |
|
Hi @ljharb -- let me know if you have any feedback / next steps on this that I can address. Sorry to ping you, but there is quite a crowd on this PR and #3227. If the issue is codecoverage, I can add new test coverage. I think my main concern was whether some of the design decisions are appropriate for the project. (just rebasing onto main & force-pushing now) |
| - 9 | ||
| - 8 | ||
| exclude: | ||
| - node-version: 17 |
There was a problem hiding this comment.
probably need to exclude node 18 as well since eslint 10 supports from node 20
https://github.com/eslint/eslint/releases/tag/v10.0.0
|
#3237 should fix the tests currently failing in |
|
@rasmi Any progress? |
|
Are there any updates ? |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Sincere apologies for the delay, can commit to a 1-day SLO on feedback/revisions until this is merged! |
rasmi
force-pushed
the
eslint-update
branch
3 times, most recently
from
8e2f0e7 to
ed9fd92
Compare
|
Thank you for working on this! Just realised I'd have to lose my clever ordering, but I'll suffer the buggy linting until this gets merged 🤞 |
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
I've rebased this. Can you pull out the pre-existing bug fixes into their own commits (ideally stacked before the eslint 10 changes)? Would also be nice to extract out the parsers refactors, including typescriptEslintParserSatisfies, to separate commits.
Also, can you elaborate on the eslint-plugin-jsonc stuff? I do see that eslint-plugin-json doesn't work on eslint 10 yet.
| })); | ||
| return; | ||
| case 'ExportAllDeclaration': | ||
| exportMap.namespace.set(specifier.exported.name || specifier.exported.value, namespace.add(exportMeta, specifier.source.value)); |
There was a problem hiding this comment.
this means it's not longer doing namespace.add - can you elaborate on this?
There was a problem hiding this comment.
namespace.add was a no-op here -- it keys off identifier.name, but was passed specifier.source.value (the module string), so it never set a getter, just returned the bare {}. Updated code sets the lazy getter directly, same as ExportNamespaceSpecifier, so export * as ns resolves.
| // Extract the base directory from the glob (everything before the first glob character) | ||
| const base = pattern.replace(/[*?{[].*/g, '').replace(/\/[^/]*$/, '') || '.'; | ||
| const resolvedBase = resolve(base); | ||
| const allFiles = walkDirectory(resolvedBase, extensions, [], fs, join, extname); |
There was a problem hiding this comment.
You can use glob if you use a version that's old enough to support the same node versions as us.
| * @param {string[]} extensions - list of supported file extensions | ||
| * @returns {string[]} list of matched file paths | ||
| */ | ||
| function listFilesWithNodeFs(src, extensions) { |
There was a problem hiding this comment.
why wouldn't this function work on eslint 9?
There was a problem hiding this comment.
It would work, just not reached as a fallback. Updated comment to clarify!
|
|
||
| test({ | ||
| code: 'const { baz } = require("./bar")', | ||
| errors: [error('baz', './bar')], |
There was a problem hiding this comment.
why are these two no longer errors?
There was a problem hiding this comment.
This is the fix from earlier. These never errored -- checkRequire early-returns unless commonjs: true, which these don't set. The errors: was presumably copied from the adjacent case in 54d86c8. Previous RuleTester just ignored it, it now flags it which is why the test failed / required a workaround in the earlier commit/comment. Will split this out in the bugfix commit!
| }), | ||
| _test({ | ||
| code: 'import("../" + name)', | ||
| errors: [dynamicImportError], |
There was a problem hiding this comment.
and why are these no longer errors?
There was a problem hiding this comment.
Same issue as above! no-dynamic-require only flags import() with esmodule: true, which these don't set, so they never errored. These were added in 7163824 and the errors was ignored by the previous RuleTester.
There was a problem hiding this comment.
(can move these into a fix commit as well)
| "eslint-plugin-import": "2.x", | ||
| "eslint-plugin-json": "^2.1.2", | ||
| "eslint-plugin-jsonc": "^2.21.1", | ||
| "espree": "^9", |
There was a problem hiding this comment.
espree is normally hoisted transitively from eslint, but installing eslint-plugin-jsonc@^3 for v10 via dep-time-travel.sh de-hoists it, breaking require.resolve('espree') in utils.js (see failed test run here). I used ^9 to avoid v11, which uses Array.prototype.at() and dies on node 12/14 (node 12 failure, node failure). v9 still works for the tests that use parsers.ESPREE.
There was a problem hiding this comment.
These become unnecessary if we use a fixture as in #3252
|
@ljharb -- ready!
The jsonc stuff is messy, we could use a fixture (see #3252) or skip the I kept If we can use a fixture to reproduce the issue in #1645 instead of |
16 participants
Summary
Add ESLint v10 support while maintaining backward compatibility with ESLint v2–v9. The v10 compat changes in shipped code use feature detection (e.g. checking if
context.languageOptionsexists) rather than version checks.Closes #3227 (ESLint v10 support) (and maybe #3079).
Split out into separate PRs (independent / pre-existing issues revealed during v10 update; this branch rebases onto them and becomes v10-only):
export * as nsre-exports under modern parsers #3250: for issues [export] incorrect report onexport * as#2002,export * as nsfalse positive on import/named rule #1883, [import/export] False "Multiple exports of name" error when two "export * as ..." contain same name. #1834, import/named and import/namespace errors after upgrading Babel #1845, False "Multiple exports of name" error when two "export * as ..." contain same name [import/export] #2289What changed
Removed API replacements
ESLint v10 removed several deprecated
contextproperties andSourceCodemethods. Each call site now feature-detects the new API and falls back to the old one:context.parserOptions→context.languageOptions.parserOptions(affectssourceType.js,childContext.js,typescript.js,scc.js,parse.js)context.parserPath→context.languageOptionsused for cache hashing instead (scc.js)context.getPhysicalFilename()→context.physicalFilenameproperty (contextCompat.js)context.getScope()→ already had a compat wrapper, but one call site indeclaredScope.jsbypassed it — now fixedsourceCode.getTokenOrCommentAfter/Before()→sourceCode.getTokenAfter/Before(node, { includeComments: true })(order.js)no-unused-modulesfile enumeration fallbackESLint v10 removed
FileEnumeratorand the internalglob-utilsmodule. The existing cascade (FileEnumerator→ legacyglob-utils) now has a third tier: a Node.jsfs+minimatchfallback that walks directories and matches globs.MODULE_NOT_FOUNDorERR_PACKAGE_PATH_NOT_EXPORTEDminimatchandis-glob(existing direct dependencies).eslintrc) sinceFileEnumeratoris no longer used. The issue remains on v9.Test infrastructure (test files only)
On v10, the unmaintained
babel-eslint(Babel 6) is replaced with@babel/eslint-parserv8 for test coverage. This restores ~475 babel-related tests that would otherwise be skipped on v10.FlatCompatRuleTesterextended to auto-inject@babel/eslint-parserconfig (requireConfigFile: false,babelrc: false, syntax plugins) and handle v10 RuleTester strictnessno-duplicatestests skipped: duplicate import identifiers correctly rejected by@babel/eslint-parser@typescript-eslint/parserv8 (import type Default, { Named }removed in TS 5.0)eslint-plugin-jsonc(with version/dependency workarounds); Replace eslint-plugin-json with a fixture processor #3252 proposes an in-repo fixture that removes themDependency changes
eslint|| ^10added to peerDeps and devDepsv10-specific parser deps are installed via
dep-time-travel.sh(not in devDeps) to avoid peer dep conflicts withtypescript@4.5.5duringnpm install:@typescript-eslint/parser@8— on all ESLint 10 Node versions-
@angular-eslint/template-parser@21— on all ESLint 10 Node versions (v13 does not support ESLint 10)@babel/eslint-parser@8+@babel/core@8(ESM, RC) — on Node >= 20^20.19.0 || >=22.12.0; v7 will not get ESLint 10 support). Babel tests are skipped; all other tests run.CI changes
eslint-8+.yml: added ESLint 10 to the matrix, excluded Node < 18dep-time-travel.sh: installs v10-specific parser deps (@typescript-eslint/parser@8and@angular-eslint/template-parser@21on all nodes,@babel/eslint-parser@8on Node >= 20 only)