Skip resolving files directly inside node_modules #52809
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
or in @types. Although the node module resolution algorithm looks for .js files there, they would never be there in correctly configured node_modules. So it should be safe to skip *.ts, *.js, *.tsx, etc. Also skips looking for files directly in node_modules/@types. Fixes microsoft#52695
typescript-bot
added
Author: Team
For Milestone Bug
sheetalkamat
approved these changes
Feb 16, 2023
andrewbranch
requested changes
Feb 16, 2023
|
It would also be good to add a test that uses tsconfig |
src/compiler/moduleNameResolver.ts
Outdated
| @@ -2839,6 +2839,12 @@ function loadModuleFromImmediateNodeModulesDirectory(extensions: Extensions, mod | |||
|
|
|||
| function loadModuleFromSpecificNodeModulesDirectory(extensions: Extensions, moduleName: string, nodeModulesDirectory: string, nodeModulesDirectoryExists: boolean, state: ModuleResolutionState, cache: ModuleResolutionCache | undefined, redirectedReference: ResolvedProjectReference | undefined): Resolved | undefined { | |||
| const candidate = normalizePath(combinePaths(nodeModulesDirectory, moduleName)); | |||
| if (moduleName.indexOf(":") > -1) { | |||
| if (state.traceEnabled) { | |||
There was a problem hiding this comment.
I think you want this if nodeModulesDirectoryExists
There was a problem hiding this comment.
Or skip if onlyRecordFailures
|
@typescript-bot test this |
|
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at b279732. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at b279732. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at b279732. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite on this PR at b279732. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at b279732. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite (tsserver) on this PR at b279732. You can monitor the build here. Update: The results are in! |
|
@DanielRosenwasser Here are the results of running the user test suite comparing Something interesting changed - please have a look. Detailswebpack
|
|
@DanielRosenwasser Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @DanielRosenwasser, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@DanielRosenwasser Here they are:Comparison Report - main..52809
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Everything looks good! |
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailsangular/angular-cli
|
|
@DanielRosenwasser Here are some more interesting changes from running the top-repos suite Detailshasura/graphql-engine
|
|
@DanielRosenwasser Here are some more interesting changes from running the top-repos suite Detailsnuxt/nuxt
|
|
@DanielRosenwasser Here are some more interesting changes from running the top-repos suite Detailsslidevjs/slidev
|
Based on PR feedbac
|
@andrewbranch I moved the node:stuff skip to tryResolve, although you should take another look because I'm not sure I put it in the right place there. |
andrewbranch
approved these changes
Feb 17, 2023
There was a problem hiding this comment.
Looks good, but since I didn’t understand @sheetalkamat’s response to my suggestion that you’ve implemented, you might want to wait for a review from her in case I missed something.
You're absolutely right that npm will never install anything that puts loose files in the |
|
Like, it's not frequent, but you can definitely find projects that make use of this old cjs resolution pattern on github (usually on the older side, ofc). (Though you have to be sneaky about how you find 'em, like looking for config files that include them - you can't use |
|
It doesn’t feel outlandish for us to say that this pattern is not recommended, but if you rely on it, simply add an entry to |
|
I'm not opposed to that - but also
I thought we already did this. #35286 |
|
@DanielRosenwasser that just made our directory iterator much more efficient (so it didn't use any extra stat calls) - it didn't swap us to using that directory iterator to prefill file existence cache entries in the module name resolver. |
|
We cache |
|
@typescript-bot run DT |
sheetalkamat
requested changes
May 4, 2023
| @@ -1,26 +0,0 @@ | |||
| // @module: commonjs | |||
There was a problem hiding this comment.
Why are we deleting this test case
There was a problem hiding this comment.
I think I deleted it by mistake instead of fixing the direct node_modules path. I restored it, although github now thinks every line changed -- maybe line endings?
First test shows that no files are resolved in node_modules itself. Second test shows that a successful resolution skips node_modules but successfully resolves anyway.
andrewbranch
approved these changes
May 10, 2023
sheetalkamat
approved these changes
May 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Although the node module resolution algorithm looks for .js files there, they would never be there in correctly configured node_modules. So it should be safe to skip *.ts, *.js, *.tsx, etc.
Fixes #52695