opensearch-project / oui

OpenSearch UI Framework
Apache License 2.0
38 stars 71 forks source link

Adding lint-staged as a dependency to reduce linting time #1077

Open DustyDogCodex opened 1 year ago

DustyDogCodex commented 1 year ago

Is your feature request related to a problem? Please describe.

In issue #1070 , it was mentioned that currently the pre-commit hook runs yarn lint for the entire codebase and not specifically for only the staged files. While working on this issue I came across a npm packaged called lint-staged which does exactly this. By using lint-staged, the lint commands only lints files that have been modified and have been staged for committing.

@joshuarrrr recommended opening this as an issue to get dev feedback on whether or not to add this as a dependency to the project. I would love to hear everyone's thoughts on whether it will be helpful using this dependency.

Describe the solution you'd like

Adding lint-staged would significantly reduce linting time by only checking files that have been modified.

Describe alternatives you've considered

@BSFishy suggested a simple script akin to the current test-staged script. I am currently working on trying to implement this and see if I can create a script that can replicate lint-staged 's features.

Additional context

NPM documentation for lint-staged: https://www.npmjs.com/package/lint-staged?activeTab=readme

BSFishy commented 1 year ago

Here is my stance: we should simply create a lint-staged script, which is similar in concept to the test-staged script, running the correct linters only for what has been staged.

My reasoning is as follows:

  1. There is already precedence for using a script rather than package with the test-staged script.
  2. By adding another dependency, even only for development, we're increasing the maintenance surface area far more than if we add another script, because that is another bit of code that is outside of our control. We need to keep up with security updates/updates in general, ensure it continues working with our tooling and any tooling we may decide to switch to in the future, etc. By using a custom script, we own all of the code and can ensure it works will all our tools while simultaneously not having to worry about security updates or the sort.
  3. Another package is additional installation time/larger download and potentially adds additional time to the runtime by doing things we don't need
  4. Creating a lint-staged script should be relatively simple, following the pattern established by test-staged, simpler than the benefit that adding the lint-staged package would have

Basically, I see the lint-staged package adding more burden than pain it's alleviating, but if there is a big selling point of it that I'm missing out on, please let me know

DustyDogCodex commented 1 year ago

hi @BSFishy ,

Thank you for your input!

The main selling point of lint-staged to me was the ease of use. Install, setup config and its good to go!

However, I agree with the points you brought up regarding the fact the this project currently already uses scripts and that this package might add more surface area for maitenance. You are correct in stating that a script might just be simpler, so I will focus on writing that next.

Cheers!