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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove dist files from top-level tsconfig to speed up eslint checks 馃弾馃殌 #4707

Merged
merged 1 commit into from Feb 24, 2020

Conversation

@bajtos
Copy link
Member

bajtos commented Feb 21, 2020

This speeds up npm run eslint from about 4m10s down to 2m50s.

More importantly, it enables proper caching behavior, so that subsequent runs of npm run eslint are super quick, even after npm run build modified dist files.

$ npm run build
(...)
$ time npm run eslint

> loopback-next@0.1.0 eslint /Users/bajtos/src/loopback/next
> node packages/build/bin/run-eslint --report-unused-disable-directives --cache .


real	0m1.626s
user	0m1.143s
sys	0m0.393s

(That's less than 2 seconds for subsequent eslint runs leveraging the cached data!)

See the discussion in #4382 for more context.

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 馃憟

This speeds up `npm run eslint` from about 4m10s down to 2m50s.

Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
@bajtos bajtos self-assigned this Feb 21, 2020
@bajtos bajtos changed the title chore: remove dist files from the top-level tsconfig chore: remove dist files from the top-level tsconfig to speed up eslint checks 馃弾馃殌 Feb 21, 2020
@bajtos bajtos changed the title chore: remove dist files from the top-level tsconfig to speed up eslint checks 馃弾馃殌 Remove dist files from top-level tsconfig to speed up eslint checks 馃弾馃殌 Feb 21, 2020
Copy link

derdeka left a comment

Great discovery!

@@ -14,6 +14,7 @@
"examples/*/node_modules/**",
"packages/*/node_modules/**",
"extensions/*/node_modules/**",

This comment has been minimized.

Copy link
@derdeka

derdeka Feb 21, 2020

Suggested change
"extensions/*/node_modules/**",
"**/node_modules/**",

BTW, any idea why this is not **/node_modules/**, ?

This comment has been minimized.

Copy link
@bajtos

bajtos Feb 21, 2020

Author Member

IDK, to be honest. I was thinking about simplifying the pattern too, but then decided to keep my changes minimal to get them landed faster.

Would you like to open a follow-up pull request to simplify this part of exclude configuration?

@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Feb 21, 2020

@bajtos Interesting. Does it hint that the patterns in https://github.com/strongloop/loopback-next/blob/master/.eslintignore have some issues?

@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Feb 21, 2020

Does it hint that the patterns in https://github.com/strongloop/loopback-next/blob/master/.eslintignore have some issues?

As far as I understand from reading the discussion in typescript-eslint/typescript-eslint#389, .eslintignore is applied by eslint only. typescript-eslint adds TypeScript to the mix, at which point tsconfig.json is consulted independently of .eslintignore. So it's important to exclude build artifacts in both files.

Quoting from typescript-eslint/typescript-eslint#389 (comment):

if you came here because your tslint/typescript setup is slow, I recommend you to try the following:

  1. Setup an .eslintignore file to ignore node_modules as well as build / non-typescript files
  2. Do the same thing for tsconfig.json via the "exclude" property to exclude node_modules or non typescript files.
@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Feb 21, 2020

@bajtos Interesting. Does it hint that the patterns in https://github.com/strongloop/loopback-next/blob/master/.eslintignore have some issues?

To actually answer your question, I believe the patterns in .eslintignore are fine, nothing to fix there.

Copy link
Contributor

dougal83 left a comment

Give that man a raise! :) Still a little shocked, had to run it three times to be sure!

@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Feb 21, 2020

Did you remove .eslintcache file before npm run eslint for the benchmark?

dougal83 added a commit to dougal83/loopback4-example-shopping that referenced this pull request Feb 22, 2020
This speeds up `npm run eslint`, good practice

see strongloop/loopback-next#4707

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
dougal83 added a commit to dougal83/loopback4-example-shopping that referenced this pull request Feb 22, 2020
This speeds up `npm run eslint`, good practice

see strongloop/loopback-next#4707

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Feb 24, 2020

Did you remove .eslintcache file before npm run eslint for the benchmark?

No. First I run npm run lint to update the cache, that took several minutes (more or less the same time as before my changes). Then I run npm run build and measured how long it takes to run npm run lint.

@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Feb 24, 2020

I am going to land this pull request as it is now, since it has been approved by many reviewers and there are no objections ("request changes" reviews).

Things we can fix in follow-up pull requests:

@bajtos bajtos merged commit d308258 into master Feb 24, 2020
5 checks passed
5 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage remained the same at 92.552%
Details
@bajtos bajtos deleted the chore/speed-up-eslint-exclude-dist branch Feb 24, 2020
dougal83 added a commit to dougal83/loopback4-example-shopping that referenced this pull request Feb 24, 2020
strongloop/loopback-next#4707 (comment)

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
dougal83 added a commit to dougal83/loopback-next that referenced this pull request Feb 24, 2020
strongloop#4707 (comment)

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
@dougal83 dougal83 mentioned this pull request Feb 24, 2020
2 of 7 tasks complete
dougal83 added a commit to dougal83/loopback-next that referenced this pull request Feb 24, 2020
strongloop#4707 (comment)

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
@dougal83 dougal83 mentioned this pull request Feb 24, 2020
3 of 7 tasks complete
dougal83 added a commit to dougal83/loopback-next that referenced this pull request Feb 24, 2020
strongloop#4707 (comment)

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
hacksparrow added a commit to strongloop/loopback4-example-shopping that referenced this pull request Feb 24, 2020
This speeds up `npm run eslint`, good practice

see strongloop/loopback-next#4707

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
hacksparrow added a commit to strongloop/loopback4-example-shopping that referenced this pull request Feb 24, 2020
strongloop/loopback-next#4707 (comment)

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
raymondfeng added a commit that referenced this pull request Feb 24, 2020
#4707 (comment)

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
raymondfeng added a commit that referenced this pull request Feb 24, 2020
#4707 (comment)

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
@bajtos bajtos mentioned this pull request Feb 25, 2020
43 of 43 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can鈥檛 perform that action at this time.