feat: implement lint-staged

[frbuceta]

lint-staged - Run linters on git staged files

Implementation of lint-staged throughout the project to upload the code without errors

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

[achrinza]

Just curious, what's the relation between this and #4330

[frbuceta]

Just curious, what's the relation between this and #4330

This PR is for using lint-staged in the Loopback project and the other (#4330) is for users to integrate lint-staged in their projects with loopback

[achrinza]

Just curious, what's the relation between this and #4330

This PR is for using lint-staged in the Loopback project and the other (#4330) is for users to integrate li, nt-staged in their projects with loopback

Ah noted, thanks for clarifying 👍

[dougal83]

How much time does this save? Linting is generally quite quick isn't it?

[frbuceta]

How much time does this save? Linting is generally quite quick isn't it?

I just did a test and it's brutal

eslint: fix(The current loopback script): 02:12.52 minutes
lint-staged: 00:09.74 minutes

lint-staged the only thing it does is pass the lint only to the files added to git, not all. It also only runs when you commit.

My intention is not to fill the project with unnecessary dependencies but this change will help us a lot

[bajtos]

Thank you @frbuceta for the pull request. I agree with you it would be great to have a faster option for running the linter.

My experience with pre-commit hooks is not the best, the hook is often run in situations when I don't want it to be executed (e.g. when performing git rebase) and thus slowing me down.

Can we please find a way how to introduce this faster way of running the linter as a new npm run script that developers can decide to use instead of npm run lint if they like to?

I love the fact that you are limiting both Prettier and Eslint to the staged files only 👍

[bajtos]

I think this is no longer needed? See https://github.com/strongloop/loopback-next/pull/4639/files

[bajtos]

eslint: fix(The current loopback script): 02:12.52 minutes

I noticed too that eslint is slow. My wild guess is that the type-based rules from @typescript-eslint package are slow and/or not correctly cached. It would be great to debug our eslint config to identify which rules are taking longest to run and then open an issue in typescript-eslint to either get the performance improved or perhaps get an advice how to speed things up.

[dougal83]

I noticed too that eslint is slow. My wild guess is that the type-based rules from @typescript-eslint package are slow and/or not correctly cached.

I think the gist is that typescript engine is adding delays. I did find an interesting post mentioning speeding up type-based rules. Does it apply to this repo?

[bajtos]

I noticed too that eslint is slow. My wild guess is that the type-based rules from @typescript-eslint package are slow and/or not correctly cached.

I think the gist is that typescript engine is adding delays. I did find an interesting post mentioning speeding up type-based rules.

Thank you @dougal83 for the link, the discussion in that issue is interesting and helpful.

Does it apply to this repo?

Did you mean the following part?

Temporary workaround for those with mixed codebases where TS is the minority: consider creating a separate config, and use package.json scripts to run the type information requiring rules only on typescript files (i.e. like "lint": "eslint -c .eslintrc.js src/**/*.js && eslint -c ts.eslintrc.js src/**/*.ts")

In loopback-next, most of the files are in TypeScript, I believe the only JavaScript files are in packages/cli - there are about 100 of such files. Compare that with more than 700 TypeScript files we have elsewhere.

I am running eslint with TIMING enabled to better understand which rules are contributing most of the slowness, will post the results once the command finishes.

$ TIMING=1 node packages/build/bin/run-eslint .

[bajtos]

Here are the results:

$ TIMING=1 node packages/build/bin/run-eslint .
Rule                                    | Time (ms) | Relative
:---------------------------------------|----------:|--------:
@typescript-eslint/no-floating-promises |  5410.678 |    47.9%
@typescript-eslint/no-misused-promises  |  2613.362 |    23.1%
@typescript-eslint/return-await         |   588.043 |     5.2%
@typescript-eslint/await-thenable       |   446.213 |     3.9%
mocha/no-identical-title                |   398.989 |     3.5%
@typescript-eslint/camelcase            |   327.698 |     2.9%
mocha/no-nested-tests                   |   309.791 |     2.7%
@typescript-eslint/no-unused-vars       |   140.790 |     1.2%
no-regex-spaces                         |   106.543 |     0.9%
no-misleading-character-class           |    66.365 |     0.6%

(Note that eslint prints only 10 slowest rules, see https://github.com/eslint/eslint/blob/b23ad0d789a909baf8d7c41a35bc53df932eaf30/docs/developer-guide/working-with-rules.md#per-rule-performance)

(UPDATE)
I takes about 4m10s to complete eslint run when the cache is disabled.

[bajtos]

I found a possible improvement: remove dist files from the tsconfig project. This will significantly improve speed gained by caching (before my change, npm run build is effectively invalidating half of the cached data). See #4707

[dougal83]

@frbuceta In light of #4707, is this PR still relevant. I was thinking that it would be great for prettier however the next release (v2) appears to have a cache option in the pipeline: https://github.com/prettier/prettier/pull/5910

[frbuceta]

@frbuceta In light of #4707, is this PR still relevant. I was thinking that it would be great for prettier however the next release (v2) appears to have a cache option in the pipeline: https://github.com/prettier/prettier/pull/5910

Correct, I am aware of that PR. I close this.