Use watch recursive directories instead of watchFile for node_modules and bower components#23484
Conversation
… and bower components
sheetalkamat
force-pushed
the
typingInstallerWatch
branch
from
78955ac to
db9620d
Compare
|
|
||
| // Get path without node_modules and bower_components | ||
| let pathToWatch = getDirectoryPath(filePath); | ||
| while (isInNodeModulesOrBowerComponents(pathToWatch)) { |
There was a problem hiding this comment.
indexof("/node_modules/")? we also have a function similar to that in getNodeModulePathParts
| return; | ||
| } | ||
| f = this.toCanonicalFileName(f); | ||
| if (isPackageOrBowerJson(f) && f !== this.toCanonicalFileName(combinePaths(this.globalCachePath, "package.json"))) { |
There was a problem hiding this comment.
the globalCache path is not going to change, we can cache that.
…stead of using loop
|
@mhegazy fixed all the comments. |
|
I'm looking, but you really want @RyanCavanaugh for the TI. |
amcasey
left a comment
amcasey
left a comment
There was a problem hiding this comment.
I thought I had understood you to say that directory watching was not reliable. Otherwise, this seems reasonable.
| } | ||
|
|
||
| function getDirectoryExcludingNodeModulesOrBowerComponents(f: string) { | ||
| const indexOfNodeModules = f.indexOf("/node_modules/"); |
There was a problem hiding this comment.
Don't we have helpers for this? E.g. commonPackageFolders in core.ts.
There was a problem hiding this comment.
But TI uses only these two locations.
|
|
||
| function isPackageOrBowerJson(fileName: string) { | ||
| const base = getBaseFileName(fileName); | ||
| return base === "package.json" || base === "bower.json"; |
There was a problem hiding this comment.
Should this take a flag for case sensitivity?
There was a problem hiding this comment.
the file here is always canonical file name
|
|
||
| watchers.set(path, createWatch(path)); | ||
| }; | ||
| const createProjectFileWatcher = (file: string): FileWatcher => { |
There was a problem hiding this comment.
Is there a reason to prefer lambdas over functions or vice versa?
There was a problem hiding this comment.
lambdas are needed because we use this
|
|
||
| watchers.isInvoked = false; | ||
| // handler should be invoked once for the entire set of files since it will trigger full rediscovery of typings | ||
| watchers.isInvoked = false; |
There was a problem hiding this comment.
This is probably a dumb question, but isn't watchers a map? Why would it have an isInvoked property?
There was a problem hiding this comment.
its a expando on map.. This is so that we arent catching incorrect local variable. the state recides on the map (more on this #23438 (comment))
|
@RyanCavanaugh can you take a look. |
4 participants
This will help in reducing polling watches.
Fixes #23390