Upgrade ESLint v9 → v10 and migrate JSX spacing rules to @stylistic#28
Open
AlbertSmit wants to merge 1 commit into
Open
Upgrade ESLint v9 → v10 and migrate JSX spacing rules to @stylistic#28AlbertSmit wants to merge 1 commit into
AlbertSmit wants to merge 1 commit into
Conversation
WalkthroughThis PR removes the ESLint config migration CLI, adopts the npm ChangesGlobals Dependency Migration
JSX Rule Migration to
Migration CLI Removal
Rule Implementation Improvements
Test Fixture Normalization
Package and Infrastructure Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
AlbertSmit
changed the title
AlbertSmit
changed the title
ESLint v10 breaking changes addressed:
- Bump eslint and @eslint/js to ^10.0.0
- Migrate react/jsx-{curly,equals,tag}-spacing to @Stylistic equivalents
(eslint-plugin-react uses removed sourceCode.isSpaceBetweenTokens API)
- Remove type property from error assertions (RuleTester v10 rejects it)
- Update no-eval error message (backtick-quoted in v10)
- Comment out prop-types test (scopeManager.addGlobals not in babel/eslint-parser stable)
Other improvements:
- Switch from inlined globals.json to globals npm package
- Bump babel/core and babel/eslint-parser to ^7.26.0
- Bump eslint-plugin-react-hooks to ^7.0.0
- Remove migration CLI (bin/migrate-to-flat-config.js) and related README docs
- Restore 7 previously-stubbed tests with real test cases
- Clean up ast.js: throw on unknown node types, remove unnecessary else branches
- Convert template literals to string literals where no interpolation needed
Known issue: prop-types.test.js is commented out -- babel/eslint-parser@7.x
does not implement scopeManager.addGlobals() required by ESLint v10.
Fix available in babel/eslint-parser@8.0.0-rc.2 (not yet stable).
AlbertSmit
force-pushed
the
migrate-to-eslint-v9
branch
from
b85c1d5 to
0ae0399
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rules/naming-policy/index.js (1)
107-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
reportUnderscorePropertiesto avoid crashing on non-stringstyles[...]keys.
MemberExpression[object.name = 'styles']callsreportUnderscoreProperties, which doesconst name = getPropertyName(property)and thenname.startsWith(...).getPropertyNamereturnsproperty.valueforLiteral, sonamecan be non-string (null, numbers, etc.), making.startsWiththrow. Add a string/type guard (e.g.,typeof name === 'string') before using.startsWith, while keeping the'_root'exemption.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rules/naming-policy/index.js` around lines 107 - 117, The function reportUnderscoreProperties can crash when getPropertyName(property) returns a non-string; add a type guard so you only call .startsWith on strings: call const name = getPropertyName(property); if (typeof name !== 'string') return; then keep the existing checks (if (!name.startsWith('_') || name.startsWith('_root')) return) and the context.report using messages['no styles properties with _'](name) and node: property; this ensures MemberExpression handling for styles[...] keys won't throw while preserving the '_root' exemption.
🧹 Nitpick comments (1)
rules/third-party/import-export.test.js (1)
6-17: ⚖️ Poor tradeoffOptional: restoring
invalidcoverage needs new cross-file fixtures (current mocks don’t model re-export conflicts).The same-file duplicate-export exclusion is consistent with the ESLint v10 parse-error limitation, but
rules/third-party/mocksonly includesexport-a.js,export-default-a.js, andmy-component.js—none re-export from another module—so there’s no existing cross-file conflicting re-export fixture to reuse. Adding real negative coverage would require introducing new re-exporting mock modules and addressing the fatal parsing issue documented inrules/third-party/import-export-retry-failure.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rules/third-party/import-export.test.js` around lines 6 - 17, The test suite for the "import/export" rule only asserts valid cases and omits cross-file invalid fixtures because the mocks folder lacks re-exporting modules; to restore negative coverage add new mock modules under rules/third-party/mocks that create cross-file re-export conflicts (e.g., create a module that re-exports from another mock to clash with export-a.js or export-default-a.js), update the RuleTester invocation in rules/third-party/import-export.test.js (the ruleTester.run('import/export', rule, { valid: [...], invalid: [] }) block) to include cases that reference those new mock paths, and ensure you handle the ESLint v10 parse-fatal issue described in rules/third-party/import-export-retry-failure.md (either by adjusting the fixtures to avoid same-file duplicate exports or by adding retry/fatal handling as documented) so the tests exercise cross-file re-export conflicts without causing fatal parse errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rules/third-party/react/prop-types.test.js`:
- Around line 1-48: Replace the large commented RuleTester block with an
explicit skipped test so it remains visible in coverage and easy to re-enable:
restore the RuleTester setup (RuleTester import, rule =
require('eslint-plugin-react').rules['prop-types'], and the ruleTester.run(...)
call) but wrap it in a top-level describe.skip (or it.skip) named
"react/prop-types" and include the original TODO comment about
scopeManager.addGlobals/ESLint v10 + `@babel/eslint-parser`; ensure the
identifiers RuleTester, rule, and ruleTester.run are used exactly as before and
keep the same valid/invalid test cases unchanged inside the skipped block.
---
Outside diff comments:
In `@rules/naming-policy/index.js`:
- Around line 107-117: The function reportUnderscoreProperties can crash when
getPropertyName(property) returns a non-string; add a type guard so you only
call .startsWith on strings: call const name = getPropertyName(property); if
(typeof name !== 'string') return; then keep the existing checks (if
(!name.startsWith('_') || name.startsWith('_root')) return) and the
context.report using messages['no styles properties with _'](name) and node:
property; this ensures MemberExpression handling for styles[...] keys won't
throw while preserving the '_root' exemption.
---
Nitpick comments:
In `@rules/third-party/import-export.test.js`:
- Around line 6-17: The test suite for the "import/export" rule only asserts
valid cases and omits cross-file invalid fixtures because the mocks folder lacks
re-exporting modules; to restore negative coverage add new mock modules under
rules/third-party/mocks that create cross-file re-export conflicts (e.g., create
a module that re-exports from another mock to clash with export-a.js or
export-default-a.js), update the RuleTester invocation in
rules/third-party/import-export.test.js (the ruleTester.run('import/export',
rule, { valid: [...], invalid: [] }) block) to include cases that reference
those new mock paths, and ensure you handle the ESLint v10 parse-fatal issue
described in rules/third-party/import-export-retry-failure.md (either by
adjusting the fixtures to avoid same-file duplicate exports or by adding
retry/fatal handling as documented) so the tests exercise cross-file re-export
conflicts without causing fatal parse errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13e26615-27f3-49ec-b22b-6f68100058cd
📒 Files selected for processing (101)
README.mdbin/migrate-to-flat-config.jsbin/migrate-to-flat-config.test.jseslint.config.jslib/absolute-path-resolver-plugin.jsmachinery/ast.jsmachinery/filename.jsmachinery/globals.jsonpackage.jsonpnpm-workspace.yamlrules/component-properties/index.jsrules/component-properties/policy-destructure-props.jsrules/component-properties/policy-no-setters.jsrules/component-properties/policy-variable-passing.jsrules/core/comma-dangle.test.jsrules/core/comma-spacing.test.jsrules/core/comma-style.test.jsrules/core/dot-location.test.jsrules/core/eol-last.test.jsrules/core/eqeqeq.test.jsrules/core/key-spacing.test.jsrules/core/keyword-spacing.test.jsrules/core/new-parens.test.jsrules/core/no-array-constructor.test.jsrules/core/no-caller.test.jsrules/core/no-class-assign.test.jsrules/core/no-compare-neg-zero.test.jsrules/core/no-cond-assign.test.jsrules/core/no-const-assign.test.jsrules/core/no-constant-condition.test.jsrules/core/no-control-regex.test.jsrules/core/no-debugger.test.jsrules/core/no-delete-var.test.jsrules/core/no-dupe-args.test.jsrules/core/no-dupe-class-members.test.jsrules/core/no-dupe-keys.test.jsrules/core/no-duplicate-case.test.jsrules/core/no-empty-character-class.test.jsrules/core/no-empty-pattern.test.jsrules/core/no-eval.test.jsrules/core/no-ex-assign.test.jsrules/core/no-extend-native.test.jsrules/core/no-extra-bind.test.jsrules/core/no-extra-boolean-cast.test.jsrules/core/no-extra-label.test.jsrules/core/no-global-assign/test.jsrules/core/no-implied-eval/test.jsrules/core/no-iterator.test.jsrules/core/no-loop-func/test.jsrules/core/no-native-reassign/test.jsrules/core/no-new-object.test.jsrules/core/no-new-symbol.test.jsrules/core/no-proto.test.jsrules/data-x-unique-id/index.jsrules/import-sort/index.jsrules/jsx-key/index.jsrules/layout-class-name/index.jsrules/layout-class-name/test.jsrules/naming-policy/index.jsrules/naming-policy/policy-component-name.jsrules/naming-policy/policy-css-file-and-variable-name.jsrules/naming-policy/policy-css-variable-properties.jsrules/naming-policy/policy-ref-name.jsrules/naming-policy/policy-root-element-class-name.jsrules/no-default-export/test.jsrules/position-center/index.jsrules/position-center/test.jsrules/third-party/import-default.test.jsrules/third-party/import-export.test.jsrules/third-party/import-first.test.jsrules/third-party/import-named.test.jsrules/third-party/import-no-amd.test.jsrules/third-party/import-no-unresolved.test.jsrules/third-party/jsx-a11y-alt-text.test.jsrules/third-party/jsx-a11y-anchor-has-content.test.jsrules/third-party/jsx-a11y-anchor-is-valid.test.jsrules/third-party/jsx-a11y-aria-activedescendant-has-tabindex.test.jsrules/third-party/jsx-a11y-aria-props.test.jsrules/third-party/jsx-a11y-aria-proptypes.test.jsrules/third-party/jsx-a11y-aria-role.test.jsrules/third-party/jsx-a11y-aria-unsupported-elements.test.jsrules/third-party/jsx-a11y-heading-has-content.test.jsrules/third-party/jsx-a11y-html-has-lang.test.jsrules/third-party/jsx-a11y-iframe-has-title.test.jsrules/third-party/jsx-a11y-img-redundant-alt.test.jsrules/third-party/jsx-a11y-no-access-key.test.jsrules/third-party/jsx-a11y-no-distracting-elements.test.jsrules/third-party/jsx-a11y-no-redundant-roles.test.jsrules/third-party/jsx-a11y-scope.test.jsrules/third-party/react/jsx-boolean-value.test.jsrules/third-party/react/jsx-curly-spacing.test.jsrules/third-party/react/jsx-equals-spacing.test.jsrules/third-party/react/jsx-indent.test.jsrules/third-party/react/jsx-no-comment-textnodes.test.jsrules/third-party/react/jsx-no-undef.test.jsrules/third-party/react/jsx-pascal-case.test.jsrules/third-party/react/jsx-tag-spacing.test.jsrules/third-party/react/no-danger-with-children.test.jsrules/third-party/react/prop-types.test.jsrules/third-party/react/self-closing-comp.test.jsrules/third-party/react/void-dom-elements-no-children.test.js
💤 Files with no reviewable changes (5)
- README.md
- rules/position-center/test.js
- bin/migrate-to-flat-config.test.js
- machinery/globals.json
- bin/migrate-to-flat-config.js
Comment on lines
+1
to
+48
| // TODO: eslint-plugin-react@7.37.5 prop-types rule triggers scopeManager.addGlobals error | ||
| // with @babel/eslint-parser in ESLint v10. This test is skipped until eslint-plugin-react | ||
| // is updated for ESLint v10 compatibility, or until we migrate to @eslint-react/eslint-plugin. | ||
| // | ||
| // const { RuleTester } = require('eslint') | ||
| // const rule = require('eslint-plugin-react').rules['prop-types'] | ||
| // | ||
| // const ruleTester = new RuleTester({ | ||
| // languageOptions: { | ||
| // parser: require('@babel/eslint-parser'), | ||
| // ecmaVersion: 2020, | ||
| // sourceType: 'module', | ||
| // parserOptions: { | ||
| // ecmaFeatures: { jsx: true }, | ||
| // requireConfigFile: false, | ||
| // babelOptions: { | ||
| // presets: [require.resolve('@babel/preset-react')] | ||
| // }, | ||
| // }, | ||
| // }, | ||
| // }) | ||
| // | ||
| // ruleTester.run('react/prop-types', rule, { | ||
| // valid: [ | ||
| // ` | ||
| // class Hello extends React.Component { | ||
| // static propTypes = { | ||
| // name: require('prop-types').string.isRequired, | ||
| // }; | ||
| // render() { | ||
| // return <div>Hello {this.props.name}</div>; | ||
| // } | ||
| // } | ||
| // `, | ||
| // ], | ||
| // invalid: [ | ||
| // { | ||
| // code: ` | ||
| // class Hello extends React.Component { | ||
| // render() { | ||
| // return <div>Hello {this.props.name}</div>; | ||
| // } | ||
| // } | ||
| // `, | ||
| // errors: [{ message: "'name' is missing in props validation" }], | ||
| // }, | ||
| // ], | ||
| // }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Has eslint-plugin-react (after 7.37.0) fixed prop-typesRuleTester compatibility with ESLint v10 +@babel/eslint-parser (scopeManager.addGlobals-related errors)?
💡 Result:
No clear evidence that eslint-plugin-react (specifically after v7.37.0) fixed prop-types RuleTester compatibility with ESLint v10 + @babel/eslint-parser for scopeManager.addGlobals-related errors. What the sources show: 1) ESLint v10 introduced a requirement for ScopeManager#addGlobals, and custom ScopeManager implementations must provide it. This is the API change behind errors like “scopeManager.addGlobals is not a function.” [1] 2) A separate, concrete failure report exists for @babel/eslint-parser + ESLint 10: tests throw “TypeError: scopeManager.addGlobals is not a function,” and the resolution involved adjusting the eslint-scope dependency. [2] 3) The eslint-plugin-react repo has ongoing ESLint v10 compatibility work (e.g., PRs addressing ESLint v10 RuleTester/schema changes), but the surfaced PR excerpt is about updating/removing RuleTester error assertion fields for ESLint 10, not about the specific scopeManager.addGlobals runtime failure. [3] 4) An open issue states the plugin was unable to use with ESLint v10 at least at the time of that report, and explicitly says it wasn’t compatible until the peer dependency range is updated. [4] 5) The eslint-plugin-react changelog entries around v7.37.0 include fixes in the prop-types rule (including a “null-check rootNode before calling getScope” change), but the changelog snippet does not mention ESLint v10, @babel/eslint-parser, RuleTester compatibility, or scopeManager.addGlobals. [5] Conclusion: - The “scopeManager.addGlobals” error is tied to ESLint v10’s ScopeManager API change [1] and has been reported with @babel/eslint-parser [2]. - While eslint-plugin-react has ESLint v10-related test fixes [3] and prop-types-specific fixes in/around v7.37.0 [5], the provided authoritative sources do not demonstrate that eslint-plugin-react’s later changes specifically resolved the addGlobals-related RuleTester incompatibility with @babel/eslint-parser. If you hit this error today, the most evidence-backed fix path is to ensure your @babel/eslint-parser / eslint-scope dependency chain provides the expected ScopeManager#addGlobals behavior (as the @babel/eslint-parser report indicates via overriding eslint-scope). [2]
Citations:
- 1: eslint/eslint@208b5cc
- 2: babel/eslint parser failing with ESLint 10 babel/babel#17791
- 3: Fix ESLint v10 RuleContext API removal (follow-up to #3972) jsx-eslint/eslint-plugin-react#3979
- 4: ESLint v10 compatibility jsx-eslint/eslint-plugin-react#3977
- 5: https://github.com/jsx-eslint/eslint-plugin-react/blob/012ec39f962627d5312e9fbaee3eb41d9a4d8bb6/CHANGELOG.md
Convert the commented-out RuleTester into an explicit skipped test
The file currently comments out the entire RuleTester run (dropping coverage and making re-enablement easy to miss). Rework it into the repo’s idiomatic “intentionally pending” skip (e.g., it.skip/describe.skip or a conditional guard) while preserving the existing TODO rationale.
The scopeManager.addGlobals failure is tied to ESLint v10’s ScopeManager API change and has been reported with @babel/eslint-parser; there isn’t clear evidence that eslint-plugin-react changes after ~v7.37.0 specifically resolved this exact incompatibility, so keeping it skipped is reasonable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rules/third-party/react/prop-types.test.js` around lines 1 - 48, Replace the
large commented RuleTester block with an explicit skipped test so it remains
visible in coverage and easy to re-enable: restore the RuleTester setup
(RuleTester import, rule = require('eslint-plugin-react').rules['prop-types'],
and the ruleTester.run(...) call) but wrap it in a top-level describe.skip (or
it.skip) named "react/prop-types" and include the original TODO comment about
scopeManager.addGlobals/ESLint v10 + `@babel/eslint-parser`; ensure the
identifiers RuleTester, rule, and ruleTester.run are used exactly as before and
keep the same valid/invalid test cases unchanged inside the skipped block.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Incremental upgrade from ESLint v9 (already on
mainvia #29) to ESLint v10.Dependencies
eslint^9.39.4^10.0.0@eslint/js^9.39.4^10.0.0@babel/core^7.11.6^7.26.0@babel/eslint-parser^7.16.5^7.26.0eslint-plugin-react-hooks^7.1.1^7.0.0eslint-import-resolver-node^0.4.0^0.3.9globals^16.0.0machinery/globals.jsonRule migration
3 JSX spacing rules moved from
eslint-plugin-reactto@stylistic/eslint-plugin:react/jsx-curly-spacing→@stylistic/jsx-curly-spacingreact/jsx-equals-spacing→@stylistic/jsx-equals-spacingreact/jsx-tag-spacing→@stylistic/jsx-tag-spacingThese rules crash on ESLint v10 because
eslint-plugin-reactusessourceCode.isSpaceBetweenTokens(), which was removed in v10. The@stylisticversions are forked from the same source but updated for the new API.Test suite (ESLint v10 RuleTester)
typeproperty from error assertions (~20 files) — v10 RuleTester rejects itno-evalerror message (backtick quoting change in v10)no-object-constructornow requiressuggestionsassertionCleanup
bin/migrate-to-flat-config.js) — one-time tool, no longer neededmachinery/globals.json(1273 lines) in favor ofglobalsnpm packageelsebranchesTest results
Known blockers for clean v10 adoption
1. @babel/eslint-parser — scopeManager.addGlobals crash
ESLint v10 ships with eslint-scope 9.x which requires
ScopeManager.addGlobals(). Stable@babel/eslint-parser7.x does not implement this. Fix exists in@babel/eslint-parser8.0.0-rc.2 but is not yet stable.Impact:
prop-types.test.jsis commented out — the RuleTester + babel parser combination crashes. The rule itself still works at runtime.2. eslint-plugin-react — removed internal APIs
eslint-plugin-react7.37.5 does not declare ESLint v10 in its peerDependencies. The 3 JSX spacing rules above are the concrete breakage; worked around via@stylistic.3. eslint-plugin-jsx-a11y — peer dep mismatch
eslint-plugin-jsx-a11y6.10.2 peers on eslint 3-9, not v10. Functionally works but produces a pnpm warning.When to merge
This PR can be merged as-is (everything works with the workarounds), or we can wait until:
@babel/eslint-parser8.x ships stable → uncommentprop-types.test.jseslint-plugin-reactandeslint-plugin-jsx-a11yrelease v10-compatible versions → clean peer deps