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

Upgrade ESLint to v9 #65648

Draft
wants to merge 19 commits into
base: trunk
Choose a base branch
from
Draft

Upgrade ESLint to v9 #65648

wants to merge 19 commits into from

Conversation

shvlv
Copy link
Contributor

@shvlv shvlv commented Sep 25, 2024

What?

Upgrade ESLint to v9.

Why?

Fixes: #64782

ESLint was moving to the new flat config and new API — https://eslint.org/docs/latest/use/migrate-to-9.0.0. The community made great progress supporting v9 — eslint/eslint#18391.

How?

The following parts were updated:

  • shared configs from eslint-plugin to support the flat format
  • custom WordPress rules to support new APi
  • lint-js command to support the v9 CLI
  • root ESLint config to support flat format
  • root package.json to upgrade ESLint plugins

Several plugins are incompatible with ESLint v9 and were fixed with eslint/compat helper:

eslint-plugin-eslint-comments was replaced by @eslint-community/eslint-plugin-eslint-comments - https://github.com/eslint-community/eslint-plugin-eslint-comments (see mysticatea/eslint-plugin-eslint-comments#79 (comment)).

Testing Instructions

Run npm run lint:js.

Current status

1766 problems (1424 errors, 342 warnings)
  33 errors and 109 warnings potentially fixable with the `--fix` option.

With upgrading plugins, new rules were introduced that have to be handled on the root config. Also, maybe the new configuration still does not completely reflect the previous one (while I strived for that goal). Some issues are repeated many times, so they can be easily addressed (like @typescript-eslint/no-explicit-any or no-undef in react-navite-editor).

Also, documentation for eslint-plugin, script, and inside /docs should be double-checked.

@gziolo gziolo added [Package] ESLint plugin /packages/eslint-plugin [Type] Breaking Change For PRs that introduce a change that will break existing functionality [Package] Scripts /packages/scripts labels Sep 25, 2024
@gziolo
Copy link
Member

gziolo commented Sep 25, 2024

Awesome work @shvlv starting this PR. Thank you so much. The number of breaking changes is overwhelming. It won't be an easy upgrade for the folks using wp-scripts lint. If they were using a custom config, an ignores file, or have some rules disabled as comments in the code, it will require a lot of manual intervention. It is what it is, and we can't do much about it other than document the migration path to make the transition easy. Let's see if we can collectively help to get it to the finish line. I invited a few more core contributors to help with review 😄

@sirreal
Copy link
Member

sirreal commented Sep 25, 2024

Are there automated migration tools that can help with migration? Were they used in this PR?

It would be helpful for folks dealing with this breaking change to document how they can migrate and what tooling is available.

@shvlv
Copy link
Contributor Author

shvlv commented Sep 26, 2024

Are there automated migration tools that can help with migration? Were they used in this PR?

Yes, tools exist.

@eslint/migrate-config

To migrate config, https://eslint.org/docs/latest/use/configure/migration-guide#migrate-your-config-file could be used. However, as the documentation says, it's appropriate chiefly for static configuration files (yml and json), but our config file has pretty complex logic, so I didn't try it. But maybe it will be a good start for wp-scripts users because I can imagine many of them have no configs or have pretty simple configs.

In projects I participated in, we often used the following approach:

const defaultConfig = require( '@wordpress/scripts/config/.eslintrc.js' );

module.exports = {
	...defaultConfig,
	settings: {
		'import/resolver': {
			typescript: {},
		},
	},
};

But yes, it will not work now for several reasons, and I'm not sure the migration tool could help either.

In general, migration to v9 is pretty complicated for several reasons:

  • you should update all ESLint plugins
  • you should find the shared config in flat format. Plugins support legacy and new formats but do it in a different way. For some plugins you can use plugin.configs.recommended, but for others it will be plugin.configs['flat/recommended'] .
  • you should understand the plugin exposes the object or the array (to be descructed)
  • in the case of an array you should iterate over array times to add custom files/ignores properties

But it's complex for the Gutenberg giant monorepo and should be easier for consuming projects.

eslint-transforms

It can be used to migrate custom rules to the new API (https://www.npmjs.com/package/eslint-transforms and https://eslint.org/blog/2023/09/preparing-custom-rules-eslint-v9/). I tried it for @wordpress/eslint-plugin; however, it shows no issues while I get errors in runtime. Hopefully, we will have a small number of custom rules, and only four must be updated.

@eslint/config-inspector

It's the new tool to make debugging easier (https://eslint.org/docs/latest/use/configure/debug#use-the-config-inspector and https://github.com/eslint/config-inspector). It doesn't work if you have config errors. But now it works like the following:

image

We can consider adding the name property to our configs to make debugging easier. I assume the tool is not so important for Gutenberg package users, but it might be helpful for internal development.

It would be helpful for folks dealing with this breaking change to document how they can migrate and what tooling is available.

Yes, we should mention these tools in the documentation (or a blog post). However, I think documentation with examples of how-to is even more important.

@shvlv
Copy link
Contributor Author

shvlv commented Sep 26, 2024

Last update:

Regarding the last one, ESLint v9 doesn't support multiple configurations as it used to. I.e., it stops after finding the parent configuration. I see the pros and cons of this decision. Pros: we have everything in a single place; it's transparent; we can merge duplicated rules. Cons: we already have 700+ lines in the config file. We can keep it as is and consider refactoring in the future. Refactoring can be done in the two directions:

  • split big configuration files into several configs splitter logically (for instance, general, package-specific, tests, etc)
  • still use the configuration on the package level, but this way, we should update our scripts to run wp-script lint:js several times: globally and additionally for specific packages (not ideal solution, perhaps)

Current status:

✖ 750 problems (407 errors, 343 warnings)
  13 errors and 110 warnings potentially fixable with the `--fix` option.

@marekdedic
Copy link
Contributor

Hi, maybe I understand things incorrectly, but it seems to me that it's still possible to have multiple config files, you just have a different mechanism for using them - importing. So each package can have its own config, which imports some shared general config much like you import and use configs from third-party plugins.

@swissspidy
Copy link
Member

This is gonna be a very painful change for the ecosystem. Is there any way we could reduce the burden for users of these packages? For example by simultaneously supporting v8 and v9 or so?

@shvlv
Copy link
Contributor Author

shvlv commented Sep 30, 2024

Hi, maybe I understand things incorrectly, but it seems to me that it's still possible to have multiple config files, you just have a different mechanism for using them - importing. So each package can have its own config, which imports some shared general config much like you import and use configs from third-party plugins.

@marekdedic, I think we are talking about different things. We have the following structure:

.eslintrc.js
package/
  react-native-editor/
    .eslintrc.js

When we run ESLint V8 from the root directory, and ESLint starts to lint files inside react-native-editor, it uses package/react-native-editor/.eslintrc.js because it's the closest file to the current working directory.https://eslint.org/docs/v8.x/use/configure/configuration-files#cascading-and-hierarchy. Even more:

ESLint then searches up the directory structure, merging any .eslintrc files it finds along the way until reaching either an .eslintrc file with root: true or the root directory.

But it does not work in this way now - https://eslint.org/docs/latest/use/configure/configuration-files#configuration-file-resolution:

When ESLint is run on the command line, it first checks the current working directory for eslint.config.js. If that file is found, then the search stops...

That means if you run eslint from the root directory and it contains the configuration file, the inner package/react-native-editor/.eslintrc.js file is ignored completely. (Also root option is not supported anymore).

@shvlv
Copy link
Contributor Author

shvlv commented Sep 30, 2024

This is gonna be a very painful change for the ecosystem. Is there any way we could reduce the burden for users of these packages? For example by simultaneously supporting v8 and v9 or so?

@swissspidy, the problem is @wordpress/eslint-plugin has a few custom rules but mostly configures third-party rules from other plugins https://github.com/WordPress/gutenberg/pull/65648/files#diff-691525462f8b4cac47c3c2e0e2668606f539ed0b3f62f8f6b79dffe7ef38bd9d. The most significant issue is typescript-eslint. To support legacy config, we should install different packages https://typescript-eslint.io/getting-started/legacy-eslint-setup, so I don't see a way to make it work 🤷‍♂️

Other issues are resolvable:

  • support legacy and new API in our custom rules
  • retain the current configs and add new ones under the flat subdirectory
  • find legacy file formats in lint-js scripts command and generate legacy or new config based on that

@marekdedic
Copy link
Contributor

@shvlv You are right, we are misunderstanding each other. :)
If you want to use a different eslint config for different packages, you either have to have one monster-config in the root, or you need to run eslint separately for each package (then either run it in the package folder or pass the config as a --config argument). I was talking about the second case, in which you can create a shared "base" config and extend it on a per-package basis.

The version 2.30.0 of `eslint-plugin-import` triggers "parserPath or languageOptions.parser is required!" error (see import-js/eslint-plugin-import#3051)
It decreases number of errors because we have `tseslint.configs.eslintRecommended` having the deal with suppressing not relevant ESLint core errors
@shvlv
Copy link
Contributor Author

shvlv commented Oct 2, 2024

Last updates:

  • eslint-plugin-import:2.3.0 doesn't detect node built-in modules correctly. The latest version 2.30.0 triggers "parserPath or languageOptions.parser is required!". So I had to update it to 2.10.0. I hope it's temporary before the new release with [Fix] ExportMap / flat config: include languageOptions in context import-js/eslint-plugin-import#3052 is created).
  • packages/block-serialization-spec-parser/shared-tests.js errors suppressed explicitly
  • enabled lining for *.d.ts files. It resolves more errors than introduces, so it makes sense while we can discuss it.

@marekdedic
Copy link
Contributor

Hi @shvlv - check out eslint 9.12, I think they may have added exactly what we were talking about last week - see eslint/eslint#18742

@shvlv
Copy link
Contributor Author

shvlv commented Oct 9, 2024

Last updates:

We are almost there:

✖ 354 problems (13 errors, 341 warnings)
  5 errors and 108 warnings potentially fixable with the `--fix` option.

@marekdedic I didn't test the last ESLint release, but from the description, I see it's exactly what we talked about. But when I'm thinking about root .eslint.config.js... There are a lot of package-specific configs. So maybe what we have now with a single root config is even better. But we should agree on the decision with someone from the maintainers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] ESLint plugin /packages/eslint-plugin [Package] Scripts /packages/scripts [Type] Breaking Change For PRs that introduce a change that will break existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[eslint-plugin] Update eslint to v9 and typescript-eslint to v8
5 participants