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

Update eslint to latest versions #334

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

KaliedaRik
Copy link
Contributor

@KaliedaRik KaliedaRik commented Dec 30, 2024

What does this change?

This PR updates the ESLint tool to the latest versions.

Note that this includes moving to ESLint's new config system - see relevant posts about this made to the ESLint blog .

The primary aim of this update is to get rid of some Dependabot security alerts, and to lower technical debt going forward.

Future Health work

  • This PR works because it is turning off (potentially essential) linting checks - eg: "no-fallthrough": "off"
  • The current output shows where we are using eslint-disable directives in files. In an ideal world, we should not need those directives.
  • Future health work should attempt to address these issues by moving each check from off to warn and then fixing the code that is causing the warn message.
  • Once those warnings are fixed, we can move the check (if essential) from warn to error to prevent anyone reintroducing such an error into the code base.

How to test

  • Run yarn lint locally - there should be plenty of warnings, but no errors reported.
  • Run the PR in the CODE environment to see if the new system brakes any CI steps.
  • Test in CODE - everything should operate as before

@KaliedaRik KaliedaRik requested review from twrichards and a team as code owners December 30, 2024 16:01
Copy link
Contributor

@rebecca-thompson rebecca-thompson left a comment

Choose a reason for hiding this comment

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

Looks good. Is it worth migrating the custom config to use @guardian/eslint-config / @guardian/tsconfig at some point?

@KaliedaRik
Copy link
Contributor Author

There's an issue with the CI build, which I can replicate locally:

  • Checkout the branch
  • Run yarn to install dependencies
  • Run yarn lint - should pass (24 warnings - all of which are Unused eslint-disable directive)
  • Run yarn build - this step fails

The reported CI failure occurs when attempting to build the client module and fails specifically because of a type-check error:

Screenshot 2025-01-07 at 16 34 43

... which I can replicate when building locally:

Screenshot 2025-01-07 at 16 39 42

@KaliedaRik
Copy link
Contributor Author

KaliedaRik commented Jan 7, 2025

Interesting aside following on from above comment. If I run yarn lint after running yarn build the lint tests report a whole bunch of fresh errors - all in the newly created dist folders:

Screenshot 2025-01-07 at 17 15 20

This doesn't affect CI because the lint step runs before the build step, but it does make me wonder what the bundler is doing to our source code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants