Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESLint v9 contains breaking API changes #3699

Open
2 tasks done
Tracked by #2961 ...
james-yeoman opened this issue Mar 4, 2024 · 60 comments
Open
2 tasks done
Tracked by #2961 ...

ESLint v9 contains breaking API changes #3699

james-yeoman opened this issue Mar 4, 2024 · 60 comments

Comments

@james-yeoman
Copy link

james-yeoman commented Mar 4, 2024

Is there an existing issue for this?

  • I have searched the existing issues and my issue is unique
  • My issue appears in the command-line and not only in the text editor

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:

  • Error: context.getScope is not a function
    • Rule: "react/no-string-refs"
  • Error: context.getFirstTokens is not a function
    • Rule: "react/display-name"

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
Copy link
Member

ljharb commented Mar 4, 2024

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
Copy link
Member

ljharb commented Mar 4, 2024

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
Copy link
Member

ljharb commented Mar 4, 2024

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
Copy link
Author

james-yeoman commented Mar 5, 2024

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
Copy link
Member

ljharb commented Mar 6, 2024

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
Copy link
Author

james-yeoman commented Apr 2, 2024

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

@ljharb
Copy link
Member

ljharb commented Apr 2, 2024

@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
Copy link

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

@ljharb ljharb changed the title [Bug]: ESLint v9 contains breaking API changes ESLint v9 contains breaking API changes Apr 6, 2024
@ljharb ljharb removed the bug label Apr 6, 2024
@nodegin
Copy link

nodegin commented Apr 8, 2024

Getting TypeError: context.getScope is not a function here

@chelsea6502
Copy link

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
Copy link
Member

ljharb commented Apr 11, 2024

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
Copy link

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).

@james-yeoman
Copy link
Author

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
Copy link
Member

ljharb commented May 28, 2024

@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
Copy link
Contributor

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
Copy link
Member

ljharb commented May 28, 2024

@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
Copy link
Author

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.

@ljharb
Copy link
Member

ljharb commented May 28, 2024

Flat config is in eslint 8, so that's not actually a reason to move to eslint v9.

@bsal649
Copy link

bsal649 commented May 28, 2024

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.

I was responding to someone who was trying to hack their way around incompatibility with eslint/compat and telling him how he could do it. I'm having trouble seeing how your puritanical views about what should/shouldn't be done hold any relevance.

We're all adults here and I'd think we're quite capable of deciding if we're happy to risk breaking eslint temporarily. I would have thought it was painfully obvious that my comment about npm crying was a joke, but I suppose not.

@ljharb
Copy link
Member

ljharb commented May 28, 2024

@bsal649 no, it isn't, even if solely because of the nonzero chance that someone could see your comment and assume that peer dep warnings are any less project-killingly dire than they are.

Pretty much every single time someone says "we're all adults here" in any context, they're drastically overestimating the capabilities and rationality of adults, and in software this is probably more true than in most contexts.

@james-yeoman
Copy link
Author

Flat config is in eslint 8, so that's not actually a reason to move to eslint v9.

What I meant was that Flat Config is the default and so doesn't require build tool checkers like esbuild-plugin-eslint to do dynamic checks in order to load it. Flat Config being the default makes it a smoother, less hacky experience.

And considering the fact that the legacy cascade config will be removed in eslint v10, eslint v9 seems to be a transitional major version. I don't personally see the benefit of maintaining the legacy cascade config while supporting eslint v9. Hence why I suggested eslint-plugin-react@7.x.x being a separate branch (i.e. legacy) while eslint-plugin-react@8.x.x being the main branch.

It wouldn't create double the workload, because the legacy branch would be for publishing major bugfixes only for eslint-plugin-react@7.x.x.

If anything, making eslint v8 and eslint v9 both work with the same version of eslint-plugin-react is doubling your workload. Breaking changes are breaking changes. If people don't understand the semantics in semver, that doesn't mean you should avoid utilising semver major versions for breaking changes.

@james-yeoman
Copy link
Author

@ljharb to be clear, I'm not trying to ignore your concerns regarding workload. I'm actively considering the balance between the dev work required, and the user experience.

If there's any nuances that are incompatible with my suggestion that I'm unaware of, I'd be grateful to hear about them, as it'd help me recognise (and better respond to) such scenarios in the future.

Even if there's no particular nuance involved and instead it's merely a matter of your personal vision, it'd be useful for it to be clarified. As I hope you can understand and empathise with, my ADHD and/or Autism encourages me to seek solutions to problems, real or not. If you simply don't want to restrict a new major version to ESLint v9+, explicit confirmation of that would allow me to shut off the problem-solving part of my brain for this.

@paralin
Copy link

paralin commented May 30, 2024

Fwiw I'm appreciating the ability to use the older config format temporarily with v9 as it is a transitional version after all. This way I can still have all my deps up to date on all projects while working towards migrating to the flat config.

@james-yeoman
Copy link
Author

james-yeoman commented May 30, 2024

@paralin You would still be able to do so, since the config loading is handled by eslint. The problem at hand is the maintenance cost for a single version of this plugin supporting eslint v8 and v9 at the same time. The incompatibilities are on the ESLint Plugin APIs end, not the config end

@ljharb
Copy link
Member

ljharb commented May 30, 2024

@james-yeoman I explicitly will not release a version of any of my eslint-related packages that drop support for eslint 8, for the foreseeable future. If a tool can't be made to support both 8 and 9, then that's a serious issue that eslint itself would need to address if it wants v9 to be adopted quickly.

@arnouxor
Copy link

i agree with @james-yeoman , you just have to write in your readme: if you want the eslint 8 version, install this version, but if you want to be up to date, install the last one. I mean, it reminds me when people wanted to support IE6 instead of moving on, i think nobody expects that from you

@ljharb
Copy link
Member

ljharb commented May 30, 2024

@arnouxor basically all my non-eslint packages still support IE 6 :-)

Regardless, eslint 9 just came out. There's simply no reason you need to update to it yet - just be patient and wait for the ecosystem to catch up, just like when eslint 8 came out, and 7, and 6, and 5, etc.

arcanis added a commit to yarnpkg/berry that referenced this issue Jun 8, 2024
## What's the problem this PR addresses?

This lets us remove the Rushstack fix, and dogfood the VSCode SDK with
the flat config (which is the only available config in Eslint 9). I
would have migrated us to Eslint 9, but eslint-plugin-react [isn't
compatible
yet](jsx-eslint/eslint-plugin-react#3699).

## How did you fix it?

Migrate to eslint.config.mjs.

## Checklist

<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [x] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests