canalplus / rx-player

DASH/Smooth HTML5 Video Player
https://developers.canal-plus.com/rx-player/
Apache License 2.0
870 stars 132 forks source link

Run eslint on the scripts directory #1529

Closed peaBerberian closed 1 month ago

peaBerberian commented 2 months ago

I noticed that the scripts directory, which begins to have a lot of files as we automate more tasks (like #1524), wasn't linted.

That directory contains code in two languages BASH and JS (Node.js), with a move to prefer JS files (as it's a JS/TS project).

So I chose to add an .eslintrc.js file (which is a deprecated format but I couldn't make the new one work) and lint script on that directory.

I subsequently fixed some linting errors.

I also added GH actions running lint and type checking when various directories are updated

peaBerberian commented 2 months ago

I think having multiple workflow for linting different parts of the project separately is a bit overkill for the current size of the project. If the goal is to speed up the lint process, I'm not sure it's relevant because linting all files already takes less than 1 minutes on the CI, and a more effective way would be to switch to a more recent lint engine like biome or oxlint.

Just to divide multiple subjects here:

  1. there's multiple .eslintrc.js files because the src directory does not obey the same rules than the demo, the tests or the scripts directory (e.g. scripts run in a Node.js environment, src in a browser, demo has react etc.)

  2. The workflows are separate not really for speed, but to avoid doing unnecessary checks for nothing and having too many actions running per PR: it already seems to be superior to what "github view" shows by default, forcing me to scroll in their interface.

    We could group all lints task under the same workflow, yet :

    • having a false negative due to an issue randomly seen in the scripts directory would be bothersome
    • We don't even need to run that script in that case, and I'm not comfortable with the idea of taking computing resources that are not even needed when that's easy to fix
    • Now that I'm thinking of speed, demo linting for example might take time where in 99% of cases it's not even needed.

I got fooled more than once by running npm run lint but it didn't catch lint errors because I had to manually run npm run lint:tests or npm run lint:demo. That would also help contributers to familiarise with the project because running only one command npm run lint is a common usage.

We could rename our npm scripts. Though in reality I run less and less eslint locally because I grew tired of its slowness, even when just checking src. But If external contributors wants to have a perfect contribution before PR-time (like I would guess most of them), sure, we could have a global npm run lint scripts and sub-scripts (what would the src script be called though, if there was one?).