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.

Sign up for GitHub

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

Jump to bottom

ESLint 9 upgrade #10731

Open
chris48s opened this issue Dec 7, 2024 · 4 comments · May be fixed by #10762
Open

ESLint 9 upgrade #10731

chris48s opened this issue Dec 7, 2024 · 4 comments · May be fixed by #10762
Labels
dependencies Related to dependency updates developer-experience Dev tooling, test framework, and CI

Comments

@chris48s
Copy link
Member

chris48s commented Dec 7, 2024 •
edited
Loading

📋 Description

This project currently uses ESLint 8.
ESLint 9 makes a big breaking change requiring the move to flat config.
For a simple project, this migration is not a huge deal. For example, I migrated most of my projects to ESLint 9 a while back. Here's a couple of examples of what the commits migrating the config looked like:

Shields is a bit more of a complicated beast though, for two reasons:

  1. The biggest issue is that Shields uses a lot of ESLint extensions. In order to migrate to ESLint 9, all the plugins and configs we use need to be compatible with ESLint 9/Flat config. Even if a lot of the extensions we use have versions which are compatible, if we are using some plugins that aren't yet compatible with ESLint 9 they may prevent us from upgrading, or we might need to look at replacing them.
  2. Shields has quite a big ESLint config https://github.com/badges/shields/blob/master/.eslintrc.yml - there is certainly more to it than the relatively simple examplesI posted above. Migrating it to the new format will be a bit more of a job, but we'll get over it.

We are now starting to see some ESLint plugins we use which now require ESLint 9. For example eslint-plugin-cypress requires ESLint 9. We're also starting to see unresolvable peerDependency conflicts with some NPM plugins.

I think the first step is to go through this list of all the eslint- packages we rely on, work out which ones do/don't have compatibility with flat config and see what the gaps are:

  • eslint-config-prettier
  • eslint-config-standard
  • eslint-config-standard-jsx
  • eslint-config-standard-react
  • eslint-plugin-chai-friendly
  • eslint-plugin-cypress
  • eslint-plugin-icedfrisby
  • eslint-plugin-import
  • eslint-plugin-jsdoc
  • eslint-plugin-mocha
  • eslint-plugin-no-extension-in-require
  • eslint-plugin-node
  • eslint-plugin-promise
  • eslint-plugin-react
  • eslint-plugin-react-hooks
  • eslint-plugin-sort-class-members

Then we can work out how many blockers we have. I don't really have a vibe for how big the gap is yet.

Annoyingly, I already know about one of the problem children here. eslint-plugin-icedfrisby does not yet have a release which is compatible with flat config, and it is maintained by.. me 😄 I've been ignoring it due to bmish/eslint-doc-generator#526 but it is probably time for me to at least revisit the topic on that repo.
@chris48s chris48s added developer-experience Dev tooling, test framework, and CI dependencies Related to dependency updates labels Dec 7, 2024
This was referenced Dec 7, 2024
Open
Open
Closed
Open
@chris48s
Copy link
Member Author

chris48s commented Dec 9, 2024

OK eslint-plugin-icedfrisby is not going to be a blocker chris48s/eslint-plugin-icedfrisby#121

@chris48s
Copy link
Member Author

Having spent a bit of time on this, the situation is not too bad.

Most of the plugins we use do have a version that supports flat config and either we're already using it or we can upgrade to it.

The exceptions are:

  • eslint-config-standard
  • eslint-config-standard-jsx
  • eslint-config-standard-react

issues:

The recommendation seems to be to migrate to https://github.com/neostandard/neostandard for those 3

eslint-plugin-no-extension-in-require is not compatible but also now that we've almost completely switched to ESM we can just get rid of this with basically no impact. We have so few require() statements left its just not an issue really.

eslint-plugin-react-hooks does have a compatible release but also there is a really long issue about it facebook/react#28313 which makes me think this one might be a case where its a bit more complicated than that we'll have to see.

..and then we've got eslint-plugin-node which I think is not compatible. But also having looked at out eslintrc, it doesn't look like we are actually using any of the rules or configs from it. I think this was something we were using in the past but are not any more so I think we could just ditch this one with no impact.

So although there are some bits of the jigsaw that need putting together, I don't think there is anything which represents a hard blocker there 🤞

@PyvesB
Copy link
Member

PyvesB commented Dec 15, 2024

we could just ditch this one with no impact

Are there any other ones we could ditch? Our eslint config has always felt very complex and obscure to me, I'd be all for simplification... :)

@chris48s
Copy link
Member Author

I'm currently knee-deep in an eslint.config.js with a crapload of TODO comments in it 😅

I agree that our eslint setup is pretty fiendish.

I think there are several things going on here:

  1. Our codebase has several distinct entities in it. The badge server, the frontend, the NPM package. Each of these has slightly different characteristics. The badge server is a node app and uses ESM. The frontend is a docusaurus/react app that runs in the browser. The NPM package is node and still uses CommonJS.

    Also, we are using several distinct test frameworks in different places for different reasons. We use mocha for the core tests. The service tests use IcedFrisby. We have end-to-end tests written in cypress, etc.

    All of that means that

    • We need (or at least choose to use) a lot of plugins
    • We need to selectively use certain bits of config in certain places

    With some simpler projects you can make a statement about the codebase as a whole like "we use react" or "the tests are written in mocha". The situation with all that stuff in this codebase is "its complicated" so there's lots of things that don't make sense to do globally. As such, we've got an ESLint setup that accounts for all that. e.g: Allow browser globals in frontend/**/*.js but node globals elsewhere. Apply mocha lint rules in **/*.spec.js but not in services/**/*.tester.js and so on ad infinitum.

    So I think in that respect our ESLint config is quite complicated, but a lot of it is there for sensible reasons.

  2. Our ESLint setup has evolved over a period of many years. There are certain bits of cruft we have accumulated over time that may now be obsolete that we could possibly remove or simplify.

  3. There are certain plugins or configs we adopted at a point where we already had a lot of code in place that maybe violates one or two of the recommended rules. In order to adopt a config without having to make massive changes everywhere, there are certain places where we had (or chose) to adopt a subset of rules in a more customised configuration rather than just go with the default.

So I think there are probably some opportunities to do a bit of cleanup and I will try and do this as part of the process, but I think on the whole we are probably still going to end up with quite a complicated config and a lot of that is just for valid reasons.

@chris48s chris48s linked a pull request Dec 20, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
dependencies Related to dependency updates developer-experience Dev tooling, test framework, and CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Migrate to ESLint 9 chris48s/shields
2 participants
@chris48s @PyvesB