Skip to content

Lint static files and dependencies with eslint + knip #23198

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

Closed
wants to merge 58 commits into from

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented Mar 20, 2025

Fixes: mozilla/addons#15482

Description

add eslint + knip + styleline to handle code style and formatting

Context

Ensure our static files are high quality with stricter linting rules. All of the changes in the ./static directory are a result of automated formatting or light manual changes when automatic formatting didn't work.

This PR will be too big to review, but you can scan the commits to see how each tool is introduced in a pattern:

  • add the tool, silencing any rules that cause errors
  • apply each rule one at a time fixing the errors

Javascript

We already used prettier for formatting, but eslint includes a more robust set of checks and extendable rules. Additionally prettier can be run via eslint, so there's still just one tool to call. Finally, knip is useful for identifying "dead code" that is not imported or used anywhere.. as you can see we had a fair bit of that.

Re-writing the config files in .ts just makes them a lot easier to validate while editing them as vscode will give you the type errors. Again, as you can see, there were invalid properties there that were previously missed.

Finally, zod gives syntax and runtime validation of our environment variables. Ensures you cannot run the build without valid parameters defined in the environment.

LESS/CSS

Using styleline I used the same approach as for JS, applied each rule one at a time to make it clear exactly what is changing why.

Testing

Test that addons-server can run addons-linter by verifying file upload works with validation as expected.

You can test the linting by:

  • breaking some css or js using invalid syntax like referencing an undefined variable in a file
  • add an npm dependency that is not used npm i react -S
  • add a js/css file that is not imported anywhere in ./static/js|css directories.

Run make lint and expect knip to show errors in each case.

Finally, you can test that knip will fix it's mistakes. After adding the additional dead code/deps run make format and knip should remove the files and dependency from package.json. The undefined variable may or may not be fixed automatically but if you "unformat" some code it should be reformatted.

Checklist

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.
  • Add before and after screenshots (Only for changes that impact the UI).
  • Add or update relevant docs reflecting the changes made.

@KevinMind KevinMind force-pushed the kevinmind/addons/15460 branch 2 times, most recently from 95af7be to d2d5c30 Compare March 20, 2025 18:45
@KevinMind KevinMind changed the base branch from kevinmind/addons/15460 to master March 20, 2025 18:46
@KevinMind KevinMind force-pushed the kevinmind/addons/15482 branch 4 times, most recently from 1f5c6dd to ae0e908 Compare March 20, 2025 19:05
@KevinMind KevinMind changed the title Kevinmind/addons/15482 Lint static files and dependencies with eslint + knip Mar 20, 2025
@KevinMind KevinMind requested review from a team and eviljeff and removed request for a team March 20, 2025 19:33
@KevinMind KevinMind force-pushed the kevinmind/addons/15482 branch 3 times, most recently from 60d568a to 2f1c508 Compare March 21, 2025 11:16
@KevinMind
Copy link
Contributor Author

Rebased after merging the vite cleanup

@KevinMind KevinMind force-pushed the kevinmind/addons/15482 branch from 2f1c508 to 05e6ae6 Compare March 21, 2025 11:26
@KevinMind KevinMind marked this pull request as draft March 21, 2025 11:46
@KevinMind KevinMind force-pushed the kevinmind/addons/15482 branch 2 times, most recently from deac710 to 52c3283 Compare March 21, 2025 18:00
@diox
Copy link
Member

diox commented Mar 24, 2025

@KevinMind wrong issue linked ?

@KevinMind KevinMind force-pushed the kevinmind/addons/15482 branch 5 times, most recently from 965ed1c to 28aaac5 Compare March 26, 2025 09:29
@KevinMind KevinMind marked this pull request as ready for review March 26, 2025 10:13
@KevinMind KevinMind force-pushed the kevinmind/addons/15482 branch from 28aaac5 to 9ba5a8a Compare March 26, 2025 11:09
@KevinMind KevinMind marked this pull request as draft March 26, 2025 11:52
@KevinMind KevinMind force-pushed the kevinmind/addons/15482 branch from 47dcbea to 58c1dfe Compare March 26, 2025 12:17
@KevinMind KevinMind force-pushed the kevinmind/addons/15482 branch from fbd213e to ba88b76 Compare March 26, 2025 15:39
@KevinMind
Copy link
Contributor Author

Rebased version #23230

@KevinMind KevinMind closed this Mar 27, 2025
@KevinMind
Copy link
Contributor Author

Closed in favor of #23229

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.

[Task]: Lint static files
2 participants