jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
8.97k stars 2.77k forks source link

ESLint v9 contains breaking API changes #3699

Closed james-yeoman closed 2 months ago

james-yeoman commented 7 months ago

Is there an existing issue for this?

Description Overview

Upon bumping to the ESLint beta for v9, I was met with several errors in my monorepo during the linting test-run.

Namely:

I get that it's still only a beta, but these API changes were announced in september 2023.

Additionally, there are some rule structure changes outlined separately that may be worth ensuring are in compliance.

Expected Behavior

Given that ESLint v9 is now in beta, I wasn't expecting to find any plugins that haven't yet addressed the API changes

eslint-plugin-react version

v7.34.0

eslint version

v9.0.0-beta.1

node version

v18.12.0

ljharb commented 7 months ago

That expectation is flawed; the point of it being in beta is for plugins to begin to address the changes - announcements are irrelevant. Thanks for the report.

A fix for this will also need to add eslint 9 into the test matrix.

ljharb commented 7 months ago

I put up a branch running tests on eslint 9; https://github.com/ljharb/eslint-plugin-react/actions/runs/8147742013/job/22269131527#step:5:21 is failing because the jsx-no-undef and jsx-uses-react and jsx-uses-vars tests call linter.defineRule. @bmish, any thoughts on an approach here?

ljharb commented 7 months ago

Additionally, when I comment out those 3 lines, i get 22166 failures because Error: ESLint configuration in rule-tester is invalid: Key "parserOptions": This appears to be in eslintrc format rather than flat config format.. Does this mean eslint 9 drops support for eslintrc, or it's just no longer the default?

If the former, how can we run the same tests with both normal eslintrc stuff on eslint 8, and also with flat config on eslint 9?

james-yeoman commented 7 months ago

ESLint 9 makes the flat config the default. It renames the ESLintRC style classes to be LegacyESLint and the Linter requires an option of {configType: "eslintrc"}, and in ESLint 10, they plan on dropping the legacy config altogether.

There's a migration guide that might be a good place to start in terms of compiling a list of required tasks for this

ljharb commented 6 months ago

Awesome, thanks for the pointers.

That will help get us unblocked for eslint 9, but we'll still have a lot of work to support eslint 10.

james-yeoman commented 6 months ago

Is there any update on this yet? Or is this still quite far off? The first release candidate for v9 released last week

ljharb commented 6 months ago

@james-yeoman it is almost never the case that all the plugins in the eslint ecosystem support a new major until awhile after the final release is out. It'd be great to beat that, but if there'd been any update, it'd be in this issue :-)

JstnMcBrd commented 5 months ago

Eslint officially released v9.0.0 today. Hope this plugin will support it soon!

nodegin commented 5 months ago

Getting TypeError: context.getScope is not a function here

chelsea6502 commented 5 months ago

I'd like to continue using this plugin, but this bug is unfortunately stopping me.

I hope we get to see a fix soon!

ljharb commented 5 months ago

Nobody’s forcing you to upgrade to eslint 9 right away ¯\_(ツ)_/¯ new eslint majors always take months before everything supports them, and this one will take longer because it’s changing the default config format.

Standard8 commented 5 months ago

and this one will take longer because it’s changing the default config format.

Having worked on a couple of other plugins, I want to elaborate that I think there's two parts to this upgrade.

The real breaking change for v9 is the fact that v9 has removed APIs - it looks like this is being covered in #3727.

The second change is flat config compatibility. Consumers can use the plugin as-is (as long as the API issues are resolved). It is slightly more work, but it is possible. Of course, supporting the flat config natively makes it easier, and ESLint has a good migration guide should anyone wanting to upgrade to v9 feel like contributing a patch (I might eventually, but I have several other plugins that I'm focussing on).

root9464 commented 5 months ago

That expectation is flawed; the point of it being in beta is for plugins to begin to address the changes - announcements are irrelevant. Thanks for the report.

A fix for this will also need to add eslint 9 into the test matrix.

@ljharb Does this mean that eslint 9 is still raw for full use?

ljharb commented 5 months ago

it means you can’t use eslint 9 with this plugin yet. It often takes many months when a new eslint major comes out before enough of the ecosystem supports it - you shouldn’t expect to upgrade right away.

graue commented 5 months ago

Thanks for your work on this plugin, @ljharb! Seems totally reasonable to take some time to support ESLint 9, and anyone who reeeeeeally needs to be on the latest and greatest should consider hiring you (if you're available for such work) or volunteering their time to make it happen. Since I'm not able to do that myself at the moment, I'm happy to wait to upgrade ESLint.

danielweck commented 5 months ago

Scott, that's a perfect answer :) Thanks all!

linusjf commented 5 months ago

The same problem occurs with eslint-plugin-solid. fyi.

TypeError: context.getScope is not a function Occurred while linting /data/data/com.termux/files/home/LearnSolidJS/FirstApp/src/FibCounter.tsx:15 Rule: "solid/reactivity" at checkForReactiveAssignment (/data/data/com.termux/files/home/LearnSolidJS/node_modules/eslint-plugin-solid/dist/rules/reactivity.js:445:69) at VariableDeclarator (/data/data/com.termux/files/home/LearnSolidJS/node_modules/eslint-plugin-solid/dist/rules/reactivity.js:768:11) at ruleErrorHandler (/data/data/com.termux/files/usr/lib/node_modules/eslint/lib/linter/linter.js:1115:48) at /data/data/com.termux/files/usr/lib/node_modules/eslint/lib/linter/safe-emitter.js:45:58 at Array.forEach () at Object.emit (/data/data/com.termux/files/usr/lib/node_modules/eslint/lib/linter/safe-emitter.js:45:38) at NodeEventGenerator.applySelector (/data/data/com.termux/files/usr/lib/node_modules/eslint/lib/linter/node-event-generator.js:297:26) at NodeEventGenerator.applySelectors (/data/data/com.termux/files/usr/lib/node_modules/eslint/lib/linter/node-event-generator.js:326:22) at NodeEventGenerator.enterNode (/data/data/com.termux/files/usr/lib/node_modules/eslint/lib/linter/node-event-generator.js:340:14) at runRules (/data/data/com.termux/files/usr/lib/node_modules/eslint/lib/linter/linter.js:1154:40)

linusjf commented 5 months ago

The same problem occurs with eslint-plugin-solid. fyi.

TypeError: context.getScope is not a function Occurred while linting /data/data/com.termux/files/home/LearnSolidJS/FirstApp/src/FibCounter.tsx:15 Rule: "solid/reactivity" at checkForReactiveAssignment (/data/data/com.termux/files/home/LearnSolidJS/node_modules/eslint-plugin-solid/dist/rules/reactivity.js:445:69) at VariableDeclarator (/data/data/com.termux/files/home/LearnSolidJS/node_modules/eslint-plugin-solid/dist/rules/reactivity.js:768:11) at ruleErrorHandler (/data/data/com.termux/files/usr/lib/node_modules/eslint/lib/linter/linter.js:1115:48) at /data/data/com.termux/files/usr/lib/node_modules/eslint/lib/linter/safe-emitter.js:45:58 at Array.forEach () at Object.emit (/data/data/com.termux/files/usr/lib/node_modules/eslint/lib/linter/safe-emitter.js:45:38) at NodeEventGenerator.applySelector (/data/data/com.termux/files/usr/lib/node_modules/eslint/lib/linter/node-event-generator.js:297:26) at NodeEventGenerator.applySelectors (/data/data/com.termux/files/usr/lib/node_modules/eslint/lib/linter/node-event-generator.js:326:22) at NodeEventGenerator.enterNode (/data/data/com.termux/files/usr/lib/node_modules/eslint/lib/linter/node-event-generator.js:340:14) at runRules (/data/data/com.termux/files/usr/lib/node_modules/eslint/lib/linter/linter.js:1154:40)

Reverted my global installation of eslint to 8.57.0. A case of TLDR;

ljharb commented 5 months ago

Indeed, you simply can't upgrade to eslint 9 until all of your eslint plugins/configs have explicit peer dep support for it. (this is true for every eslint major)

olaf-cichocki commented 5 months ago

Hey, the reason for error (and backward's compatible implementation) can be found here: https://eslint.org/blog/2023/09/preparing-custom-rules-eslint-v9/#context.getscope()

As for sourceCode.getFirstTokens() it's here: https://eslint.org/blog/2023/09/preparing-custom-rules-eslint-v9/#from-context-to-sourcecode

lucasprag commented 5 months ago

Also getting the error for these rules:

    'react/no-direct-mutation-state': 0,
    'react/require-render-return': 0,
    'react/no-string-refs': 0,
    'react/jsx-uses-react': 0,
    'react/react-in-jsx-scope': 0,
    'react/no-danger-with-children': 0,
    'react/jsx-no-undef': 0,
    'react/jsx-uses-vars': 0,
sethlivingston commented 5 months ago

Thanks for all your hard work on this, maintainers.

Until support for eslint v9 is ready, you can create a fresh v8 config with:

npm install -D eslint@8
npm init @eslint/config@0
alaa-paramount commented 4 months ago

Hi, I'm listing all the problematic rules that I faced when migrated to eslint 9 with the eslint-plugin-react:

react/no-string-refs react/display-name react/no-direct-mutation-state react/require-render-return react/jsx-no-undef react/jsx-uses-vars react/no-typos react/no-array-index-key react/prefer-stateless-function react/no-access-state-in-setstate react/jsx-fragments react/jsx-max-depth react/jsx-no-constructed-context-values react/no-unstable-nested-components react/function-component-definition react/destructuring-assignment react/no-unused-prop-types

Standard8 commented 4 months ago

I'm not a maintainer here, but listing the details of what's failing is not going to help this move forward. The list will be obvious to anyone running the tests with v9. They effectively amount to "me too" comments that are only really serving to spam everyone with email.

If you look around, you'll see there is ongoing work towards v9 by contributors - #3743 and #3727. Be patient or help out. Adding extra comments here isn't going to help.

saltycrane commented 4 months ago

I found the ESLint Compatibility Utilities worked for me. CodeSandbox example: https://codesandbox.io/p/devbox/billowing-tree-wp33ds

npm install --save-dev @eslint/compat

eslint.config.mjs:

import { fixupConfigRules } from "@eslint/compat";
import reactRecommended from "eslint-plugin-react/configs/recommended.js";
import globals from "globals";

export default [
  ...fixupConfigRules(reactRecommended),
  {
    languageOptions: {
      globals: {
        ...globals.browser,
      },
      parserOptions: {
        ecmaFeatures: {
          jsx: true,
        },
      },
    },
    rules: {
      "react/react-in-jsx-scope": "off",
    },
    settings: {
      react: {
        version: "detect",
      },
    },
  },
];

package.json:

{
  "scripts": {
    "lint": "eslint"
  },
  "dependencies": {
    "globals": "^15.2.0",
    "react": "^18",
    "react-dom": "^18"
  },
  "devDependencies": {
    "@eslint/compat": "^1.0.1",
    "eslint": "^9.2.0",
    "eslint-plugin-react": "^7.34.1"
  }
}
onlywei commented 4 months ago

Even with the use of @eslint/compat, installation remains an issue. Shall we continue to install eslint-plugin-react using npm install --force?

ljharb commented 4 months ago

@onlywei no, you should continue to hold off upgrading to eslint 9 until all of the eslint-related packages you use support it.

karlhorky commented 4 months ago

I found the ESLint Compatibility Utilities worked for me.

@saltycrane thanks for your comment above, this was helpful!

For anyone who is not using the recommended config but rather just configuring the plugin and its rules manually, [the fixupPluginRules compatibility util](https://eslint.org/blog/2024/05/eslint-compatibility-utilities/#:~:text=Then%2C%20use%20the%20fixupPluginRules()%20function%20in%20your%20eslint.config.js%20file%20to%20wrap%20the%20plugin%20in%20a%20compatibility%20layer%3A) from @eslint/compat may be what you're looking for.

First, install @eslint/compat as a dev dependency. Then, use fixupPluginRules in your config:

eslint.config.mjs

import { fixupPluginRules } from '@eslint/compat';
import react from 'eslint-plugin-react';
import globals from 'globals';

export default [
  {
    languageOptions: {
      globals: {
        ...globals.browser,
      },
      parserOptions: {
        ecmaFeatures: {
          jsx: true,
        },
      },
    },
    plugins: {
      react: fixupPluginRules(react),
    },
    rules: {
      // Error on creating components within components
      // https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-unstable-nested-components.md
      'react/no-unstable-nested-components': 'error',
    },
    settings: {
      react: {
        version: 'detect',
      },
    },
  },
];
mdjermanovic commented 4 months ago

@ljharb it looks like the code changes have been merged, and the only task left is to make the rule tests work with ESLint v9?

ljharb commented 4 months ago

@mdjermanovic those were just the deprecations, which are the easy part. next we need tested flat config support, and then figuring out how to test on eslint 9 since we need to simultaneously test on eslintrc in a bunch of older versions (unfortunately since RuleTester doesn’t obey the eslintrc env var, this work can’t be delayed until after shipping v9 support)

MirKml commented 4 months ago

Little notice, as I know, VS Code support for flat config isn't still stable. So even when all plugins will be upgraded, without stable IDE support eslint 9 isn't very usefull (for my colleagues 🙂)

mdjermanovic commented 4 months ago

how to test on eslint 9 since we need to simultaneously test on eslintrc in a bunch of older versions (unfortunately since RuleTester doesn’t obey the eslintrc env var, this work can’t be delayed until after shipping v9 support)

I'll submit a PR for rule tests, per https://github.com/jsx-eslint/eslint-plugin-react/pull/3743#issuecomment-2083367121.

ljharb commented 4 months ago

@MirKml it'll likely be 6 months minimum before eslint 9 is made fully useful by the majority of the ecosystem supporting it; it normally takes 2-3 months for any eslint major, but eslint 9 is a much more disruptive change, so it'll probably take longer.

adamspotlite commented 4 months ago

@karlhorky have you found a way to make this typesafe?

I'm using something like:

...
import tseslint from "typescript-eslint";

...
export default tseslint.config({ ... });

but getting yelled at because:

          Type 'FixupRuleDefinition' is not assignable to type 'LooseRuleDefinition'.ts(2322)

EDIT: For anyone wondering about typesafety as a result of this, please see this issue in the @eslint/compat repo.

bsal649 commented 4 months ago

@adamspotlite here's what I did, works without issues so far (aside from npm crying about peer deps).

devDeps:

"devDependencies": {
    "@eslint/compat": "^1.0.0",
    "@eslint/js": "^9.2.0",
    "@types/eslint__js": "^8.42.3",
    "@types/node": "^20.7.0",
    "@types/react": "^18.3.0",
    "@types/react-dom": "^18.3.0",
    "eslint": "^9.3.0",
    "eslint-plugin-react": "^7.34.0",
    "eslint-plugin-react-hooks": "rc",
    "globals": "^15.1.0",
    "typescript": "next",
    "typescript-eslint": "rc-v8"
  }

npm update --force

eslint.config.js:

import eslint from '@eslint/js';
import { fixupPluginRules } from '@eslint/compat';
import globals from 'globals';
import reactPlugin from 'eslint-plugin-react';
import reactHooksPlugin from 'eslint-plugin-react-hooks';
import tseslint from 'typescript-eslint';

export default tseslint.config(
  // extends ...
  eslint.configs.recommended,
  ...tseslint.configs.strictTypeChecked,
  ...tseslint.configs.stylisticTypeChecked,
  {
    files: ['**/*.ts', '**/*.tsx'],
    languageOptions: {
      globals: globals.node,
      parserOptions: {
        project: true,
        tsconfigRootDir: import.meta.dirname,
      },
      parser: tseslint.parser,
    },
    plugins: {
      ['react']: fixupPluginRules(reactPlugin),
      ['react-hooks']: reactHooksPlugin,
      ['@typescript-eslint']: tseslint.plugin,
    },
    rules: {
      ...reactPlugin.configs['jsx-runtime'].rules,
      'react-hooks/exhaustive-deps': 'warn', // Add the rule for enforcing useEffect dependencies
      '@typescript-eslint/explicit-module-boundary-types': 'warn', // Add the rule for enforcing explicit return types on functions
    },
  },
  {
    extends: [tseslint.configs.disableTypeChecked],
    files: ['**/*.js'],
  },
);

fixupConfigRules did not work for me however, but afaik all it does is add one setting to language options (below) and loads the rules (which we are doing anyway). Not really sure how necessary this language option is, but you can add it yourself if you find yourself troubleshooting:

languageOptions: Object.assign({}, all.languageOptions, {
    parserOptions: Object.assign({}, all.languageOptions.parserOptions, {
      jsxPragma: null, // for @typescript/eslint-parser
    }),
  }),

If anyone's wondering, I only included my other eslint dependencies for completeness. I don't think any of them would be necessary (except typescript-eslint if you're using typescript, and this should be on rc-v8).

ljharb commented 4 months ago

To be clear, peer dep warnings aren’t “crying”, they’re “it might happen to work sometimes, but if it doesn’t it’s your own fault, this is the danger zone”, and they should be taken very seriously.

bsal649 commented 4 months ago

If you have your own quality tests, then it shouldn't be a significant issue. You take on a risk everytime you update a package, it's not necessarily much worse. Especially if you know all the breaking changes in the peer dependency and check them yourself.

Also, I figured out how to make npm stop crying and wailing like a little baby. Just add this to package.json:

    "eslint-plugin-react": "^7.34.0",
    "eslint-plugin-react-hooks": "rc",
    "globals": "^15.1.0",
    "prettier": "^3.2.5",
    "typescript": "^5.4.0",
    "typescript-eslint": "rc-v8"
  },
  "overrides": {
    "eslint": "^9.3.0"
  }
ljharb commented 4 months ago

Again, it's a significant issue even if only because you're violating the conditions under which it's reasonable to expect support. Your repeated violations of the code of conduct doesn't make you any more correct. Continue it and you'll be banned from this organization.

root9464 commented 4 months ago

and when is it planned to support "eslint v9" in the plugin of the same name in vscode? because at the moment eslint supports current up to v8.57 and new .mjs configs.js doesn't see it and throws an error.

MirKml commented 4 months ago

@root9464 This is little off-topic. This plugin doesn't depend on any vscode extension. When eslint-plugin-react will be ready to work with eslint v9 on the command line, job's done.

Discuss your question please with the extension developers in their github repo.

bsal649 commented 4 months ago

Again, it's a significant issue even if only because you're violating the conditions under which it's reasonable to expect support. Your repeated violations of the code of conduct doesn't make you any more correct. Continue it and you'll be banned from this organization.

Could you explain to me how I have violated your code of conduct?

MariuzM commented 4 months ago

Again, it's a significant issue even if only because you're violating the conditions under which it's reasonable to expect support. Your repeated violations of the code of conduct doesn't make you any more correct. Continue it and you'll be banned from this organization.

Could you explain to me how I have violated your code of conduct?

I also don't understand where the issue is, from my perspective @bsal649 is contributing with a solution so not sure why it's marked as abuse?

dmathisen commented 4 months ago

@MariuzM and @bsal649 His comments are being removed because they're hacks and completely unhelpful. His suggestions were to npm update --force and to add overrides. That is completely against what the purpose of NPM is and it's unsafe to do that. Library authors don't want to force their users to put hacks into place that can break other packages or break in the future. Neither of the proposed "solutions" are good solutions. They're shitty hacks and are completely unacceptable answers to the issue at hand.

This guy has mentioned that npm is "crying" about incompatibility issues. That's a feature, not a bug. This guy has a fundamental misunderstanding of how npm and libraries, in general, are supposed to work.

james-yeoman commented 4 months ago

since we need to simultaneously test on eslintrc in a bunch of older versions

@ljharb Why does a single version need to support both formats? Why not have a major version release that only supports v9 (as a breaking change)?

ljharb commented 4 months ago

@james-yeoman because major bumps (breaking changes) are the single largest cost any maintainer can expose on the ecosystem, far far far larger than maintaining compatibility, and i don’t wish to impose that cost.

james-yeoman commented 4 months ago

I'm not sure I understand. Isn't the whole point of major bumps in semver to denote breaking changes? It's not like people will be forced to upgrade to eslint-plugin-react@8.x.x, and eslint-plugin-react@7.x.x could still receive bugfixes. We would just need to have 7.x.x and 8.x.x on separate branches.

There's every chance I'm ignorant/uneducated to the difficulties with releases for popular OSS projects. In which case, I'd greatly appreciate an outline of the difficulties that would arise.

ljharb commented 4 months ago

@james-yeoman yes, when a breaking change occurs, a major bump is how you communicate it. I'm saying that it's best to not make breaking changes at all.

So, your suggestion is that I, an unpaid volunteer open source maintainer, should double my workload by maintaining a second release line for the foreseeable future, all so people can use a slightly newer linter a few months sooner?

lukeapage commented 4 months ago

He’s suggesting dropping support for old eslint versions eg a lot less work for you- no maintenance on old eslint versions and no extra complexity to handle testing both versions.

You are taking a stand that a) it has to be perfect for the community b) therefore I delay it, making it painful for the community.

a far more normal/pragmatic approach is to say any new feature/bugfixes require eslint v9.

ljharb commented 4 months ago

@lukeapage I have a lot of experience that tells me that dropping support actually creates more work than the virtually nonexistent work of maintaining support over a long time period.

This plugin isn't anywhere close to the lone holdout in supporting either flat config or eslint v9. Every time eslint releases a major version, it takes the ecosystem months to support it, and this is the most disruptive major they've ever done, so it'll likely take much longer.

Pragmatism to me would suggest that there's simply no reason anyone needs to rush into using eslint 9, and patience would be the least painful path.

james-yeoman commented 4 months ago

So, your suggestion is that I, an unpaid volunteer open source maintainer, should double my workload

That's not quite what I was suggesting, but I can see why it would be interpreted that way.

I was suggesting that the v7 branch be kept for bug fixes for however long you wish to maintain support for ESLint v8 (i.e. a legacy branch). Considering ESLint v10 will remove support for the legacy config format, it doesn't make much sense (to me at least) maintaining both formats in the same branch.

The only downside I can see from this perspective is that it might not work with the ESLint v9 environment flag that enables the legacy format. Depending on your views, this could be a dealbreaker. However, at that point, you could argue that if you need to use the legacy format, then stay on ESLint v8. The benefit of ESLint v9 is the flat config itself.