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

Migrate to ESLint 9 #10762

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Migrate to ESLint 9 #10762

wants to merge 9 commits into from

Conversation

chris48s
Copy link
Member

Closes #10731

  • Upgrade to ESLint 9
  • Upgrade all ESLint configs and plugins to versions supporting ESLint 9/ Flat config
  • Migrate Standard JS --> NeoStandard
  • Remove plugins we don't want/need any more
  • Migrate config from legacy to flat format

This PR is a bit of a monster.

Normally I'd try and make small scoped commits so it is easier to understand what is going on at each stage of the process, but in this PR there were so many moving parts changing at once it wasn't really possible to do that.

Because so much stuff was all changing at once and we also wanted to simplify where possible, I've taken a holistic look at the config and rebuilt it from the ground up. My objective was to arrive at something that replicates the spirit of what we started with (you'll notice there are not major code changes being made in this PR to account for lots of lint rule changes), but I wasn't aiming to arrive at a line by line replica of what we had before. There are several reasons why I've chosen to make changes rather than migrating things exactly, including:

  • There were some things we had set explicitly in the past, but they're now the default.
  • There were some aspects of our config that are now outdated in some way, or where keeping it is of limited value. Some examples of this include
    • vestigial config relating to the pre-docusaurus react/typescript frontend and
    • config that was needed on older node versions but is now obsolete.
  • There were some things that were in some way misconfigured to start with (an example of this is applying some mocha-related settings to the whole codebase rather than just test files).
  • There were breaking changes to account for in some of the package upgrades.
  • The overrides/customisations we need for neostandard were slightly different to the old standard js plugins

Anyway, the upshot of all this is that the new config is not a faithful recreation of the old one and the PR is probably quite hard to review, but the actual outcome should be an improvement 🤞

Another thing to note is: Our new ESLint config is in some ways even more verbose than the old one. There are certainly a lot more lines of code in it. That said, I'm hoping that one of the things I've managed to do here is make it clearer and more logically separated into logical units. So hopefully it is a bit easier to read and understand, even if it is longer.

Fixes
'overrideLogoSize' is never reassigned. Use 'const' instead
These comments came from a swizzled upstream
component but never did anything in our codebase.

ESLint 9 does not allow disable comments
for rules that are not registered.
These were here because in the past we were applying
mocha lint rules to files which contained no tests

ESLint 9 now flags eslint-disable comments
that aren't doing anythings
ESLint 9 now flags eslint-disable comments
that aren't doing anything
and update the handful of files violating it
@chris48s chris48s added the developer-experience Dev tooling, test framework, and CI label Dec 20, 2024
Copy link
Contributor

Warnings
⚠️ This PR modified service code for node but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for npm but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for snapcraft but not its test code.
That's okay so long as it's refactoring existing code.
Messages
📖 ✨ Thanks for your contribution to Shields, @chris48s!
📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS against bd83ea3

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire file is likely to need a rebase to resolve conflicts before we can merge. I'll come back to this once we have something approved

Comment on lines +234 to +242
globalConfig,
commonJsConfig,
nodeEsmConfig,
frontendConfig,
servicesConfig,
mochaConfig,
cypressConfig,
serviceTestsConfig,
jsDocConfig,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I've structured this, I've written lots of small ESLint Config objects and then composed them all at the end here.

If we wanted, we could actually split these all out into individual files. I haven't done it here, but I am happy to do it if we think it would be useful.

Comment on lines +97 to +100
rules: {
'react-hooks/rules-of-hooks': 'error',
'react-hooks/exhaustive-deps': 'error',
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have migrated to docusaurus, we don't really have much custom react code any more although there is a little bit.
Looking over our old config, the only rule from eslint-plugin-react we actually had enabled was react/jsx-sort-props so I decided to just ditch eslint-plugin-react completely.
I think there is some value in keeping these two rules because mistakes with hooks are common causes of infinite loops

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Dev tooling, test framework, and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESLint 9 upgrade
1 participant