jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
8.86k stars 2.75k forks source link

[New] support eslint v9 #3759

Open mdjermanovic opened 1 month ago

mdjermanovic commented 1 month ago

Refs https://github.com/jsx-eslint/eslint-plugin-react/issues/3699

Adds utility to automatically transform tests to ESLint v9 format when ESLint v9 is used.

I have all tests still passing with ESLint v8 locally (we'll see what happens with the older versions in CI).

Around ~500 tests are failing with ESLint v9 due to new RuleTester checks. Some because of problems in tests (e.g., duplicate tests or missing some now-mandatory test case properties), some because of problems in rules (e.g., no-op options schema or unsubstituted placeholders in messages). I'll try to fix that in separate commits in this PR.

Also fixed bugs that ESLint v9 RuleTester caught in rules jsx-closing-bracket-location and no-invalid-html-attribute.

mdjermanovic commented 1 month ago

CI with ESLint v9 fails here:

******> npm ls >/dev/null
npm error code ELSPROBLEMS
npm error invalid: eslint@9.3.0 /home/runner/work/eslint-plugin-react/eslint-plugin-react/node_modules/eslint
eslint-plugin-react@7.34.1 /home/runner/work/eslint-plugin-react/eslint-plugin-react
├── @babel/core@7.24.5
├── @babel/eslint-parser@7.24.5
├── @babel/plugin-syntax-decorators@7.24.1
├── @babel/plugin-syntax-do-expressions@7.24.1
├── @babel/plugin-syntax-function-bind@7.24.1
├── @babel/preset-react@7.24.1
├── @types/eslint@7.2.10
├── @types/estree@0.0.52
├── @types/node@4.9.5
├── @typescript-eslint/parser@5.62.0
├── array-includes@3.1.8
├── array.prototype.findlast@1.2.5
├── array.prototype.flatmap@1.3.2
├── array.prototype.toreversed@1.1.2
├── array.prototype.tosorted@1.1.3
├── aud@2.0.4
├── babel-eslint@10.1.0
├── doctrine@2.1.0
├── es-iterator-helpers@1.0.19
├── eslint-config-airbnb-base@15.0.0
├── eslint-doc-generator@1.7.1
├── eslint-plugin-eslint-plugin@5.5.1
├── eslint-plugin-import@2.29.1
├── eslint-remote-tester-repositories@1.0.1
├── eslint-remote-tester@3.0.1
├── eslint-scope@3.7.3
├── eslint@9.3.0 invalid: "^3 || ^4 || ^5 || ^6 || ^7 || ^8" from the root project
├── espree@3.5.4
├── estraverse@5.3.0
├── glob@10.3.7
├── istanbul@0.4.5
├── jackspeak@2.1.1
├── jsx-ast-utils@3.3.5
├── ls-engines@0.8.1
├── markdownlint-cli@0.32.2
├── minimatch@3.1.2
├── mocha@5.2.0
├── npmignore@0.3.1
├── object.entries@1.1.8
├── object.fromentries@2.0.8
├── object.hasown@1.1.4
├── object.values@1.2.0
├── prop-types@15.8.1
├── resolve@2.0.0-next.5
├── semver@6.3.1
├── sinon@7.5.0
├── string.prototype.matchall@4.0.11
├── typescript-eslint-parser@20.1.1
└── typescript@3.9.10

npm error A complete log of this run can be found in: /home/runner/.npm/_logs/2024-05-19T19_26_11_701Z-debug-0.log
got status code 1
1

Seems I'll have to update peerDependencies in root package.json in this PR to make it reach the test step?

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.62%. Comparing base (417e1ca) to head (ef6f9c5).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3759 +/- ## ========================================== - Coverage 97.79% 97.62% -0.17% ========================================== Files 134 134 Lines 9613 9615 +2 Branches 3486 3486 ========================================== - Hits 9401 9387 -14 - Misses 212 228 +16 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ljharb commented 1 month ago

yes, you'll need to update the peerDeps and devDeps ranges to include eslint 9.

mdjermanovic commented 1 month ago

In https://github.com/jsx-eslint/eslint-plugin-react/pull/3759/commits/d8f31860a7b3198bf2ef5a180c11a7d61ccf13c5, https://github.com/jsx-eslint/eslint-plugin-react/pull/3759/commits/9a86bc84ced7ce3232c8ac7d7f101c27a44f3415 & https://github.com/jsx-eslint/eslint-plugin-react/pull/3759/commits/85eed3279f724f76742e8a585d12a4532240f9f3, I updated several tests where output was the same as code to use output: null to assert that there's no autofix expected. ESLint v9 RuleTester reports output === code as a possible error (by writing the full output code, it looks like the author is expecting an autofix). If you want, you can doublecheck whether in those test cases it is really expected that there is no autofix.

socket-security[bot] commented 1 month ago

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/eslint@9.3.0 environment Transitive: eval, filesystem, shell, unsafe +82 9.6 MB eslintbot

View full report↗︎

mdjermanovic commented 1 month ago

yes, you'll need to update the peerDeps and devDeps ranges to include eslint 9.

Done in https://github.com/jsx-eslint/eslint-plugin-react/pull/3759/commits/a1402a2d624f740ffc1a35ce03abaf3522ecebff & https://github.com/jsx-eslint/eslint-plugin-react/pull/3759/commits/cda54a334d82e8d440f254179680e53105c3b705. I expected the dev deps update to be problematic for linting since this project uses eslintrc config, but npm still installs ESLint v8 (I guess because of the plugins' peer deps) so it's okay. I can now see in CI the same tests that are failing for me locally with ESLint v9. Working on the fixes.

mdjermanovic commented 1 month ago

Actually, it isn't okay everywhere in CI. pretest fails with:

Oops! Something went wrong! :(

ESLint: 9.3.0

ESLint couldn't find an eslint.config.(js|mjs|cjs) file.

https://github.com/jsx-eslint/eslint-plugin-react/actions/runs/9157980135/job/25175410426?pr=3759#logs

mdjermanovic commented 1 month ago

All checks are green now, except for linting because it installs ESLint v9.

Perhaps we could remove ESlint v9 from devDeps and update .github/workflows/node-18+.yml to install ESLint with --save-dev instead of --no-save?

https://github.com/jsx-eslint/eslint-plugin-react/blob/8e1a94b67d081fdc132e9a7e175db3fbf2e02956/.github/workflows/node-18%2B.yml#L43-L48

ljharb commented 1 month ago

For 5801156c39469050595f83cf1ce145d720b00f6e, can we use something like messageId, so the text doesn't have to be duplicated?

mdjermanovic commented 1 month ago

For 5801156, can we use something like messageId, so the text doesn't have to be duplicated?

Sure, now I updated all tests to use messageId in https://github.com/jsx-eslint/eslint-plugin-react/pull/3759/commits/17c4df7495eb54b3a745bb4289e31f24b06e0f12. I was using desc only because the existing tests were using desc too.

Forgot to note about this change: ESLint v9 RuleTester requires that suggestion test objects must have messageId or desc specified.

mdjermanovic commented 1 month ago

I see some commits from this branch are already merged into master. Anything else I could do to help finish eslint v9 support?

I think the only remaining issue here was a technical problem of how to ensure that eslint v8 is used for linting this project (https://github.com/jsx-eslint/eslint-plugin-react/pull/3759#issuecomment-2121476913).

ljharb commented 1 month ago

I've rebased this, and we'll see what tests are still failing.

mdjermanovic commented 1 month ago

For me locally, npm installs eslint v8.

But in pretest CI, it installs eslint v9. Maybe because of this:

https://github.com/jsx-eslint/eslint-plugin-react/blob/417e1ca292788c75618dc994b084c3a57c483fce/.github/workflows/node-pretest.yml#L15-L16

Perhaps we could remove it?

ljharb commented 1 month ago

aha. if we remove it i suspect npm install will fail, but it's worth a shot!

mdjermanovic commented 1 month ago

All the checks are green, except for pending codecov statuses, which it seems will not arrive.

ljharb commented 1 month ago

Indeed; codecov is broken rn (https://github.com/codecov/feedback/issues/374) so we don't have to worry about that.

How can we also test that this plugin works in someone else's flat config?

DysektAI commented 1 month ago

@ljharb Is it possible to upload this to npm with a tag for others to test? If so I am sure many users would be willing to test it with their setup and report any issues or problems, if not the other option is to create multiple configs and test them manually or automate it which might still miss some variations and be quite time consuming.

ljharb commented 1 month ago

@DysektAI since this package doesn't have a build process, you can just npm install the PR - npm i jsx-eslint/eslint-plugin-react#pull/3759, i believe :-)

DysektAI commented 1 month ago

So far in my testing everything works fine as it should even while using a few other plugins and a bunch of rules no errors or issues using the flat config and VS Code ESLint extension others will probably need to test and ensure consistency but looks good on my end no hacks or customization needed to make it work like react-hooks which requires using this currently and the canary version

image

eslint.config.js ``` import { fixupPluginRules } from "@eslint/compat"; import js from '@eslint/js'; import react from 'eslint-plugin-react'; import hooks from 'eslint-plugin-react-hooks'; import globals from "globals"; export default [ { name: "base", files: ["**/*.js", "**/*.jsx"], ignores: ["node_modules/**"], languageOptions: { ecmaVersion: "latest", sourceType: "module", globals: { ...globals.browser, }, parserOptions: { ecmaFeatures: { jsx: true, }, }, }, linterOptions: { reportUnusedDisableDirectives: "error", noInlineConfig: true, }, plugins: { react, 'react-hooks': fixupPluginRules(hooks) }, rules: { 'react/jsx-uses-react': 'error', 'react/jsx-uses-vars': 'error', ...js.configs.recommended.rules, ...hooks.configs.recommended.rules, // Possible Errors "for-direction": "error", "getter-return": "error", "no-async-promise-executor": "error", "no-await-in-loop": "warn", "no-compare-neg-zero": "error", "no-cond-assign": "error", // Best Practices "accessor-pairs": "warn", "array-callback-return": "error", "block-scoped-var": "error", "class-methods-use-this": "warn", "complexity": ["warn", 11], // Adjust the threshold as needed // Variables "no-delete-var": "error", "no-label-var": "error", "no-restricted-globals": ["error", "event", "fdescribe"], "no-shadow": "error", "no-shadow-restricted-names": "error", // Stylistic Issues "array-bracket-newline": ["error", "consistent"], "array-bracket-spacing": ["error", "never"], "array-element-newline": ["error", "consistent"], "block-spacing": ["error", "always"], "brace-style": ["error", "1tbs", { "allowSingleLine": true }], // ECMAScript 6 "arrow-body-style": ["error", "as-needed"], "arrow-parens": ["error", "as-needed"], "arrow-spacing": ["error", { "before": true, "after": true }], "generator-star-spacing": ["error", { "before": false, "after": true }], "no-confusing-arrow": "error", "react/jsx-key": "error", // Enforce unique keys for elements in arrays "react/no-unescaped-entities": "error", // Prevent unescaped HTML entities in JSX "react/prop-types": "off", // Disable if using TypeScript "react/no-unused-state": "error", // Warn when state variables are unused // Additional recommended rules: "react/jsx-no-useless-fragment": "error", // Avoid unnecessary fragments "react/self-closing-comp": "error", // Enforce self-closing tags "react/button-has-type": "error", // Enforce type attribute for buttons "react/no-unknown-property": "error", // Prevent usage of unknown DOM properties "react/jsx-boolean-value": ["error", "never"], // Disallow unnecessary boolean value literals in JSX "react/jsx-curly-brace-presence": ["error", "never"], // Enforce consistent use of curly braces in JSX "react/jsx-no-duplicate-props": "error", // Disallow duplicate props in JSX }, settings: { "react": { "version": "detect" } } } ]; ```
mdjermanovic commented 1 month ago

How can we also test that this plugin works in someone else's flat config?

I've added a test setup and one example in https://github.com/jsx-eslint/eslint-plugin-react/pull/3759/commits/ef6f9c5dbaca93f45b19e763e73b585a9161ac3a. Let me know if this looks good, and I can add examples with plugin configs (recommended, all, jsx-runtime) as well.