Skip to content

Fix incorrect string match on node_modules #33339

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

Closed
wants to merge 1 commit into from
Closed

Fix incorrect string match on node_modules #33339

wants to merge 1 commit into from

Conversation

danyq
Copy link

@danyq danyq commented Sep 10, 2019

ignoredPaths is used in a string match, so the dot is unnecessary and causes it to never match.

Fixes #33338
Related to #22826
Related to #31248
Related to #33335

@danyq
Copy link
Author

danyq commented Sep 10, 2019

I think tsc.js and tsserver.js are generated from this file and may need to be regenerated.

@danyq
Copy link
Author

danyq commented Sep 10, 2019

It looks like the tests are expecting node_modules to be in watchedDirectories, etc, and I suspect that's incorrect. Can anyone confirm?

@AviVahl
Copy link

AviVahl commented Sep 10, 2019

I'm guessing the code aimed to ignore node_modules/.cache and such. Not the entire node_modules, which contains .d.ts files which are part of the compilation (and it can be argued whether they should be watched or not).

@danyq
Copy link
Author

danyq commented Sep 10, 2019

You're right, it looks like older code had the function isPathInNodeModulesStartingWithDot.

Watching node_modules is definitely questionable: ours has >136k files of which .cache is 2.6k. The number of watches for a normal project easily exceeds the default limit on some systems, and we suspect that it's related to other performance issues, like those mentioned above.

I'll close this though, and hope someone who knows better can address the issue.

@danyq danyq closed this Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tsserver watches node_modules directory unnecessarily
2 participants