Tweak module resolution failed lookup watching #53591
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
sheetalkamat
force-pushed
the
resolutionDir
branch
16 times, most recently
from
6e32417 to
0e3a48d
Compare
sheetalkamat
force-pushed
the
resolutionDir
branch
6 times, most recently
from
9d88990 to
73ddbad
Compare
…g "/" even from os root directory
…ive of allowJs files
sheetalkamat
force-pushed
the
resolutionDir
branch
from
73ddbad to
71ff774
Compare
sheetalkamat
changed the title
sheetalkamat
requested review from
DanielRosenwasser,
andrewbranch,
jakebailey and
RyanCavanaugh
| @@ -205,6 +206,30 @@ export function removeIgnoredPath(path: Path): Path | undefined { | |||
| path; | |||
| } | |||
|
|
|||
| function perceivedOsRootLengthForWatching(pathComponents: Readonly<PathPathComponents>, length: number) { | |||
There was a problem hiding this comment.
This logic is same as before except it returns number of components that are roots for watching. Like c:\users\someUserName or in case of linux /anything/anything etc. So we would never watch anything in those directories.
| if (length === undefined) length = pathComponents.length; | ||
| // Ignore "/", "c:/" | ||
| // ignore "/user", "c:/users" or "c:/folderAtRoot" | ||
| if (length <= 2) return false; |
There was a problem hiding this comment.
This is just fast fail as root would alreast be 1
| dir: failedLookupSplit.slice(0, rootSplitLength + 1).join(directorySeparator), | ||
| dirPath: failedLookupPathSplit.slice(0, rootSplitLength + 1).join(directorySeparator) as Path | ||
| }; | ||
| return getDirectoryOfFailedLookupWatch(failedLookupComponents, failedLookupPathComponents, Math.max(rootPathComponents.length + 1, perceivedOsRootLength + 1)); |
There was a problem hiding this comment.
This helps with not watching "/users" when "/" is root directory (math.max part)
|
Truthfully I'm a little out of my element in trying to review this; is this something I could just build locally and try the |
|
Yes . This should fix npm ci on our repo |
|
@typescript-bot pack this |
|
Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at 5b0cbcc. You can monitor the build here. |
|
Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
It seems like it works, but, testing TS at nightly and stable, I can't actually get the bad behavior on |
|
If we don't have a repro for Typescript itself, is this still needed? From the description it sounds like the new way is strictly better even if it doesn't unblock us personally. |
|
Oh, I don't have a problem with taking this of course, I'm just annoyed that I seemingly forgot what it was I was doing that this fixed |
|
@jakebailey i think it will repro if you host typescript at |
|
Oh, huh. I've never checked out TS at that level before, so, would this not apply to the issue that I was having with But, trying it out at that level, and it definitely breaks before and works afterward. So, it's definitely an improvement! |
|
@andrewbranch do you mind taking a look at this so i can get this in. |
|
Merging for now and will fix feedback from @andrewbranch in another PR so its in nightly and can make it into beta. |
This simplifies unnecessary extension watching from long ago when we had different resolutions based on .ts and .js files
Before we would never watch directories like
c:/typescript/node_modulesor/user/username/typescript/node_modulesbecause we didnt watch folders likec:/typescriptor/user/username/typescript. Now changed to watch node_modules in root folder. (We wont watch it at root unless tsconfig is at the root)This should help with some of the npm ci issues in repo encountered if typescript repo was at root or in another folder .