Drop eslint-plugin-react - #2466
Merged
Merged
Conversation
`eslint-plugin-react@7.37.5` declares support for ESLint `^9.7` at most, and 7.37.5 is the latest release -- the `next` tag is still `7.8.0-rc.0`. That makes it the one thing preventing this repo from moving to ESLint 10, and with it `@cloudfour/eslint-config` v26, whose peer range is `^10.6.0`. See #2458, which cannot even `npm ci` today. We used the plugin for exactly one rule. `react/jsx-uses-vars` marks JSX-referenced identifiers as used, which our .mdx docs pages need: import { Canvas, Meta } from '@storybook/addon-docs/blocks'; import * as AlertStories from './alert.stories.js'; <Meta of={AlertStories} /> `no-unused-vars` does not count a JSX reference as a use, so without that rule it reports every one of those imports -- 171 errors across 83 files, measured by removing the plugin and running the linter. Turning `no-unused-vars` off for .mdx files covers the same ground without the dependency. The trade-off is that a genuinely unused import in an .mdx file stops being reported; these are documentation pages, so that seems a cheap price for unblocking the upgrade. `eslint-plugin-react-x` was considered as a replacement and rejected: at 5.18.6 it has no `jsx-uses-vars` equivalent, so it is not a drop-in. Closes #2464
|
✅ Deploy Preview for cloudfour-patterns ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
8 tasks
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.
Overview
eslint-plugin-react@7.37.5supports ESLint^9.7at most, and that is the latest release — thenexttag is still7.8.0-rc.0. It is the one thing keeping this repo on ESLint 9, and therefore off@cloudfour/eslint-configv26, whose peer range is^10.6.0. #2458 currently cannot even get throughnpm cibecause of it.We used the plugin for exactly one rule.
react/jsx-uses-varsmarks JSX-referenced identifiers as used, which our.mdxdocs pages rely on — they import Storybook's doc components and their own stories module, then reference them only inside JSX.no-unused-varsdoes not count a JSX reference as a use, so without that rule it reports every one of those imports. I measured it by removing the plugin and running the linter: 171 errors across 83 files.Turning
no-unused-varsoff for.mdxfiles covers the same ground without the dependency. The trade-off is that a genuinely unused import in an.mdxfile stops being reported — these are documentation pages, so that seems a cheap price for unblocking a major upgrade.eslint-plugin-react-xwas considered as a replacement and rejected: at 5.18.6 it has nojsx-uses-varsequivalent, so it isn't a drop-in.This does not upgrade ESLint itself. It only removes the blocker, so #2458 can proceed once #2465 (TypeScript 6) lands too.
Screenshots
Testing
npm cifollowed bynpm run preprocess, thennpm run lint:check— it should pass with no errorssrc/components/alert/alert.mdx, and confirm theCanvas/Metaimports at the top are not reported as unused.tsor.jsfile and confirmnpm run lint:checkstill catches it there — the change should only affect.mdxnpm run typeandnpm run build, both of which should succeednpm startand click through a few component docs pages in Storybook — the Canvas, Controls and Meta blocks should all render as beforeNote on
npm test: 5 of the 12 test files fail locally for me, all browser tests erroring with "Failed to fetch dynamically imported module". They fail identically on unmodifiedmain, so they are unrelated to this change — worth confirming they behave the same in CI.Closes #2464
Unblocks #2458