Upgrade eslint to 9 #21918

diedexx · 2024-12-13T17:03:01Z

Context

Our eslint config package was in need of a refresh, both in contents and in syntax (now ESLint is pushing harder for flat configs.
We had some deprecated or unnecessary rules.
The preset made sense for react applications, but required non-react projects to also set a react version, which is a bit odd.

Summary

This PR can be summarized in the following changelog entry:

[eslint-config-yoast 1.0.0 Enhancement] Update the eslint-config-yoast package to export a flat configuration file and remove the old format.
Splits off more specific configuration objects that can be combined for JS/React/Node
Updates all our JS packages to use the new flat ESLint configuration.
Upgrades ESLint to v9 for all packages.
Removes/Replaces deprecated ESLint rules.
Speeds up linting JS.
Removes the need for eslint-disable max-len for translation strings.

Relevant technical choices:

Tip

Reviewer note
This PR is best reviewed commit-by-commit.
Please read trough the commit messages.

The branch starts with cleaning up the old config, to help make sense of it all.

Removing unnecessary rules (disabled or already included in other presets we use)
Grouping them by source and category
replacing 1 and 2 with their respective labels (warn and error)

After that focus shifts to moving everything in this repo over to a flat config.

I had to update all consumers to flat config, since extending a flat config in a eslintrc file didn't seem to work. And since we'll have to move to flat configs eventually, I chose not to dive much deeper into making it eslintrc-compatible.
I found that flat mjs configs are supported as of ESLint 8.57, so I set it as the minimum version in the peer dependency alongside 9 and up. I've tested this to work with this version range.
In some rules we specified options that matched the default. I removed them. If we ever notice that a default changes when we upgrade and we don't like the new default, we can always specify what we need. Otherwise, let ESLint and its plugins find sensible defaults.
I decided to add some defaults for our tests in the rules in commonly used places. The impact for projects not using Jest this is negligible.
I decided to let max-len ignore strings. This is almost always the case with long translation strings, which need to be on the same line anyway.
The JSDoc/require-jsdoc rule seems to have gotten more aggressive. It gave way more errors on (arrow)functionExpressions. These are often not used as API and are mostly fine leaving without JSDoc, though that's up to the devs and reviewers now.
I left any package that didn't have a yarn lint script alone, with the exception of the eslint package, where I added the lint script.
I introduce a rule where prototype-builtins are not allowed. It is part of the recommended js config from ESLint. There's no reason why we shouldn't adhere to it. I made it a warning and increased the max-warning limit of projects where we break this rule.

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

Check CI. the JSLint job should succeed.
Checkout the code locally and run yarn lint in the project root. This should succeed. (albeit with a couple of warnings within the max-warning threshold for some packages)

Relevant test scenarios

Changes should be tested with the browser console open
Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
Changes should be tested on different editors (Default Block/Gutenberg/Classic/Elementor/other)
Changes should be tested on different browsers
Changes should be tested on multisite

Test instructions for QA when the code is in the RC

QA should use the same steps as above.

QA can test this PR by following these steps:

N/A - possibly a quick smoke test of our analysis in the editor and some settings pages.

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

UI changes

This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Other environments

This PR also affects Shopify. I have added a changelog entry starting with [shopify-seo], added test instructions for Shopify and attached the Shopify label to this PR.

Documentation

I have written documentation for this change. For example, comments in the Relevant technical choices, comments in the code, documentation on Confluence / shared Google Drive / Yoast developer portal, or other.

Quality assurance

I have tested this code to the best of my abilities.
During testing, I had activated all plugins that Yoast SEO provides integrations for.
I have added unit tests to verify the code works as intended.
If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
I have written this PR in accordance with my team's definition of done.
I have checked that the base branch is correctly set.

Innovation

No innovation project is applicable for this PR.
This PR falls under an innovation project. I have attached the innovation label.
I have added my hours to the WBSO document.

Fixes the "Deprecated rules" part of https://github.com/Yoast/lingo-other-tasks/issues/455

All that are removed are already part of the eslint/js/recommended shared configuration. There is one exception: no-native-reassign. It is deprecated and has been replaced with no-global-assign, which is part of the recommened config.

The Yoast configuration isn't responsible for rules other configs migh have enabled. This also cleans up our config a bit, keeping the focus on things we want to enforece. Some of these rules are interesting to us to enforce in the future. I'll create a new issue to track that.

no-shadow is its official replacement and caches the same problems

This doesn't change any of the rules, only the order in which they are in the config. This makes it easier to find details about a certain rule and understand its origin and purpose.

This is more clear than the integer values, but changes nothing else

As recommended by https://eslint.org/docs/latest/extend/shareable-configs#publishing-a-shareable-config

Now it's a JS file, we can and should adhere to our own standards

This is what we already use in most of our codebases. This moves that decision up one level into the shared config.

wp i18n, which we use for our translations, doesn't handle strings that span multiple lines. That's why we often add eslint ignore comments to those. By ignoring strings, we make a tradeoff: we no longer have to eslint-disable those translations, but we won't get feedback on long non-translation strings that might have been better readable on separate lines.

This could be relevant when doing something like this: expect( new MyValueObject( null ) ).toThrow(...);

We have serveral cases where complexity is too high. To prevent us from adding eslint-ignore lines, we make this a warning, so we can set the max-warning threshlold. This will allow us to fix these cases gradually, while not losing track of them because they are ignored.

We disable this rule in a lot of projects already. Most of the time these expressions can do without a bulky jsdoc block. Let's leave it up to the reviewer to identify the exceptions where more context is needed.

Also lint all js, instead of just src. This requires some very minor fixes.

github-actions · 2024-12-16T07:12:03Z

@diedexx Please be aware that following packages have been abandoned and are not actively maintained anymore:

Package name	Path
@yoast/babel-preset	packages/babel-preset
@yoast/components	packages/components
@yoast/e2e-tests	packages/e2e-tests
@yoast/helpers	packages/helpers
@yoast/jest-preset	packages/jest-preset
@yoast/style-guide	packages/style-guide

Please consider using the other packages instead.

github-actions · 2024-12-24T02:58:54Z

A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch.

…slint-to-9

packages/eslint/package.json

packages/eslint/default.mjs

mykola · 2024-12-26T01:08:54Z

@diedexx really impressed work to upgrade all rules and packages.
I have added some questions.

When I run locally I get the following error with the current branch:

Also I have checked premium - we use eslint-config-yoast there:
"eslint-config-yoast": "link:./vendor/yoast/wordpress-seo/packages/eslint",

So I guess when we merge current PR we'll have eslint issues there so I guess it's better to upgrader premium in scope of this ticket as well.

diedexx · 2024-12-27T08:24:41Z

Thanks @mykola!

Oh good shouts!

I hadn't pulled premium-configuration in locally. I'll check that one out right now.
The premium plugin I'd rather lock on the npm released v6 right now and upgrade it later, not to create more hurdles we need to clear before we can merge this. I'll open a PR.

diedexx · 2024-12-27T09:32:49Z

I opened two very small PRs (1,2) and pushed one commit 3c88cc4. Could you help met get those merged @mykola?

github-actions · 2025-01-06T14:38:28Z

A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch.

…slint-to-9

github-actions · 2025-01-07T07:21:07Z

@diedexx Please be aware that following packages have been abandoned and are not actively maintained anymore:

Package name	Path
@yoast/babel-preset	packages/babel-preset
@yoast/components	packages/components
@yoast/e2e-tests	packages/e2e-tests
@yoast/helpers	packages/helpers
@yoast/jest-preset	packages/jest-preset
@yoast/style-guide	packages/style-guide

Please consider using the other packages instead.

github-actions · 2025-01-08T09:08:30Z

@mhkuu Please be aware that following packages have been abandoned and are not actively maintained anymore:

Package name	Path
@yoast/babel-preset	packages/babel-preset
@yoast/components	packages/components
@yoast/e2e-tests	packages/e2e-tests
@yoast/helpers	packages/helpers
@yoast/jest-preset	packages/jest-preset
@yoast/style-guide	packages/style-guide

Please consider using the other packages instead.

diedexx added 30 commits December 13, 2024 15:43

Remove redundant options that match the rule's defaults

6bec3d5

Remove redundant and deprecated rule

76e41fb

no-shadow is its official replacement and caches the same problems

Replace deprecated rules with their suggested replacement

7b935bb

Group plugin rules together

e457c63

This doesn't change any of the rules, only the order in which they are in the config. This makes it easier to find details about a certain rule and understand its origin and purpose.

Replace 1 and 2 with warn and error

929705c

This is more clear than the integer values, but changes nothing else

Update URLs refering to the old location of the package

4bae9a2

Add plugins that the config uses as dependencies

9ff8a34

As recommended by https://eslint.org/docs/latest/extend/shareable-configs#publishing-a-shareable-config

Update shared preset to a flat config

61c5adb

Lint the config file itself

7cf8202

Now it's a JS file, we can and should adhere to our own standards

Split off React specific config in a separate preset

b5406a4

Split off Node specific config in a separate preset

f1838a1

Bake in support for Jest-based tests

a2c54e3

Let the consumer define language options

8adb43c

Let the React preset detect the version

6d2dc54

Add type docs to config

2ef531e

Tweak jsDoc rule settings

a9ddba1

Prefer @const over @constant

7f49404

This is what we already use in most of our codebases. This moves that decision up one level into the shared config.

Allow for testing side-effects of a constructor in tests

e8929d4

This could be relevant when doing something like this: expect( new MyValueObject( null ) ).toThrow(...);

Allow for omitting jsdoc on (arrow) function expressions

f10ca9f

We disable this rule in a lot of projects already. Most of the time these expressions can do without a bulky jsdoc block. Let's leave it up to the reviewer to identify the exceptions where more context is needed.

Update helpers to a flat eslint config

0935820

Update social-metadata-previews to a flat eslint config

a3acb5a

Update ui-library to a flat eslint config

a9f515f

Also lint all js, instead of just src. This requires some very minor fixes.

Update yoastseo to a flat eslint config

2757e3c

Autofix ESLint in yoastseo

8aebacc

Fix remaining ESLint errors in yoastseo

ab34229

Update social-metadata-forms to a flat eslint config

906cbf1

Restore rule that prevents having to build all js before linting

49c0bf3

diedexx marked this pull request as ready for review December 16, 2024 07:47

diedexx requested a review from a team as a code owner December 16, 2024 07:47

Remove empty lines left by the autofixer

39fbcbd

Yoast deleted a comment from github-actions bot Dec 16, 2024

github-actions bot added the merge conflict label Dec 24, 2024

Merge branch 'trunk' of github.com:Yoast/wordpress-seo into upgrade-e…

950d53e

…slint-to-9

github-actions bot removed the merge conflict label Dec 24, 2024

Yoast deleted a comment from github-actions bot Dec 24, 2024

mykola reviewed Dec 26, 2024

View reviewed changes

packages/eslint/package.json Outdated Show resolved Hide resolved

packages/eslint/default.mjs Show resolved Hide resolved

Bump the eslint-config-yoast version

3c88cc4

Yoast deleted a comment from github-actions bot Dec 27, 2024

github-actions bot added the merge conflict label Jan 6, 2025

Merge branch 'trunk' of github.com:Yoast/wordpress-seo into upgrade-e…

5c8b085

…slint-to-9

github-actions bot removed the merge conflict label Jan 7, 2025

Yoast deleted a comment from github-actions bot Jan 7, 2025

Fix newly introduced warnings after merge

bb46094

mhkuu closed this Jan 8, 2025

mhkuu reopened this Jan 8, 2025

mykola merged commit 0c73abc into trunk Jan 8, 2025
34 checks passed

mykola deleted the upgrade-eslint-to-9 branch January 8, 2025 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade eslint to 9 #21918

Upgrade eslint to 9 #21918

diedexx commented Dec 13, 2024 •

edited by mhkuu

Loading

github-actions bot commented Dec 16, 2024

github-actions bot commented Dec 24, 2024

mykola commented Dec 26, 2024

diedexx commented Dec 27, 2024 •

edited

Loading

diedexx commented Dec 27, 2024

github-actions bot commented Jan 6, 2025

github-actions bot commented Jan 7, 2025

github-actions bot commented Jan 8, 2025

Upgrade eslint to 9 #21918

Upgrade eslint to 9 #21918

Conversation

diedexx commented Dec 13, 2024 • edited by mhkuu Loading

Context

Summary

Relevant technical choices:

Test instructions

Test instructions for the acceptance test before the PR gets merged

Relevant test scenarios

Test instructions for QA when the code is in the RC

Impact check

UI changes

Other environments

Documentation

Quality assurance

Innovation

github-actions bot commented Dec 16, 2024

github-actions bot commented Dec 24, 2024

mykola commented Dec 26, 2024

diedexx commented Dec 27, 2024 • edited Loading

diedexx commented Dec 27, 2024

github-actions bot commented Jan 6, 2025

github-actions bot commented Jan 7, 2025

github-actions bot commented Jan 8, 2025

diedexx commented Dec 13, 2024 •

edited by mhkuu

Loading

diedexx commented Dec 27, 2024 •

edited

Loading