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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make type checking lint rules faster - Part II #1324

Closed
samreid opened this issue Sep 8, 2022 · 7 comments
Closed

Make type checking lint rules faster - Part II #1324

samreid opened this issue Sep 8, 2022 · 7 comments

Comments

@samreid
Copy link
Member

samreid commented Sep 8, 2022

This is an offshoot of #1238, but that issue was getting too long so I created a new issue.

I observed a staggering 55 second precommit hook yesterday and wanted to double check on performance. Reading https://typescript-eslint.io/docs/linting/troubleshooting, I decided to try:

Index: eslint/tsconfig.eslint.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/eslint/tsconfig.eslint.json b/eslint/tsconfig.eslint.json
--- a/eslint/tsconfig.eslint.json	(revision 2a475c94191876a8888830f094e19139b9d9798d)
+++ b/eslint/tsconfig.eslint.json	(date 1662644486655)
@@ -1,6 +1,3 @@
 {
-  "extends": "../../chipper/tsconfig/all/tsconfig.json",
-  "include": [
-    "../../*/**/*.ts"
-  ]
+  "extends": "../../chipper/tsconfig/all/tsconfig.json"
 }
\ No newline at end of file

In doing so, I saw the time go from 22 seconds down to 7 seconds:

~/apache-document-root/main/gravity-and-orbits$ time grunt lint --disable-eslint-cache
Running "lint" task

Done.

real	0m22.336s
user	0m25.896s
sys	0m2.786s
~/apache-document-root/main/gravity-and-orbits$ time grunt lint --disable-eslint-cache
Running "lint" task

Done.

real	0m7.383s
user	0m11.399s
sys	0m0.747s
@samreid samreid self-assigned this Sep 8, 2022
@samreid
Copy link
Member Author

samreid commented Sep 8, 2022

With the smaller, faster config, I tried triggering a type-checking lint rule and a non-type-checking lint rule, and I saw that both were correctly caught (in gravity-and-orbits). Next I'm running lint-everything to see if this works for every repo.

Things are mostly going well, but molecule-polarity hangs and reports 83 errors like this after 2.5 minutes:

/Users/samreid/apache-document-root/main/molecule-polarity/js/twoatoms/view/TwoAtomsViewProperties.ts
  0:0  error  Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser.
The file does not match your project config: ../molecule-polarity/js/twoatoms/view/TwoAtomsViewProperties.ts.
The file must be included in at least one of the projects provided

✖ 83 problems (83 errors, 0 warnings)

The problem was that it was missing from the tsconfig.

samreid added a commit that referenced this issue Sep 8, 2022
samreid added a commit to phetsims/brand that referenced this issue Sep 8, 2022
samreid added a commit to phetsims/perennial that referenced this issue Sep 8, 2022
@samreid
Copy link
Member Author

samreid commented Sep 8, 2022

In the commits, I got a speedup by about 2x by removing extraneous includes. But this meant I needed to list things specifically in the tsconfig/all/tsconfig.json.

@samreid
Copy link
Member Author

samreid commented Sep 8, 2022

@jessegreenberg can you please review?

@jessegreenberg
Copy link
Contributor

Nice, that is a big improvement! I will be out of office for a few days but will try to review soon.

@jessegreenberg
Copy link
Contributor

Lint is working well for me after this change, I tried linting everything without cache. Type checking lint rules work great.

Reporting my results on Windows - before these changes (chipper at 2a475c9) I see lint taking ~30 seconds. On master lint is taking ~20 seconds. It is faster!

I see individual files added to 944c43e. Would it be better to add their containing directories or maybe up to js/**/*? Mostly I am just thinking that adding new individual files to the tsconfig will be forgotten. Will it be loud and the errors clear that adding new files/directories to the tsconfig is required?

jessegreenberg added a commit that referenced this issue Sep 12, 2022
@samreid
Copy link
Member Author

samreid commented Sep 21, 2022

Will it be loud and the errors clear that adding new files/directories to the tsconfig is required?

The errors will be loud and clear. If lint discovers a typescript file that is not covered by the tsconfig, it will report it like this. I renamed ArithmeticModel.js => ArithmeticModel.ts and ran lint. It said:

/Users/samreid/apache-document-root/main/arithmetic/js/common/model/ArithmeticModel.ts
  0:0  error  Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser.
The file does not match your project config: ../arithmetic/js/common/model/ArithmeticModel.ts.
The file must be included in at least one of the projects provided

I feel comfortable leaving tsconfig somewhat sparse where possible, and we can add patterns as we proceed. Sound OK?

@samreid samreid assigned jessegreenberg and unassigned samreid Sep 21, 2022
@jessegreenberg
Copy link
Contributor

That seems very clear to me. OK thanks! Closing.

samreid added a commit to phetsims/perennial that referenced this issue Oct 21, 2024
samreid pushed a commit to phetsims/perennial that referenced this issue Oct 21, 2024
samreid added a commit to phetsims/perennial that referenced this issue Oct 22, 2024
zepumph pushed a commit to phetsims/perennial that referenced this issue Oct 22, 2024
zepumph pushed a commit to phetsims/perennial that referenced this issue Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants