Refactor module resolution Extensions, fix lookup priorities
#51471
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
typescript-bot
added
Author: Team
For Uncommitted Bug
andrewbranch
force-pushed
the
bug/module-resolution-refactor
branch
from
579a4a7 to
ba45a79
Compare
andrewbranch
commented
Nov 9, 2022
Comment on lines
-87
to
+93
| enum Extensions { | ||
| TypeScript, /** '.ts', '.tsx', or '.d.ts' */ | ||
| JavaScript, /** '.js' or '.jsx' */ | ||
| Json, /** '.json' */ | ||
| TSConfig, /** '.json' with `tsconfig` used instead of `index` */ | ||
| DtsOnly, /** Only '.d.ts' */ | ||
| TsOnly, /** '.[cm]tsx?' but not .d.ts variants */ | ||
| const enum Extensions { | ||
| TypeScript = 1 << 0, // '.ts', '.tsx', '.mts', '.cts' | ||
| JavaScript = 1 << 1, // '.js', '.jsx', '.mjs', '.cjs' | ||
| Declaration = 1 << 2, // '.d.ts', etc. | ||
| Json = 1 << 3, // '.json' | ||
|
|
||
| ImplementationFiles = TypeScript | JavaScript, | ||
| } |
There was a problem hiding this comment.
Extensions is bit flags now, and TSConfig is changed to be an isConfigLookup on ModuleResolutionState.
Comment on lines
+1607
to
-1594
| // ./foo.js -> ./foo.ts | ||
| const resolvedByReplacingExtension = loadModuleFromFileNoImplicitExtensions(extensions, candidate, onlyRecordFailures, state); | ||
| if (resolvedByReplacingExtension) { | ||
| return resolvedByReplacingExtension; | ||
| } | ||
|
|
||
| // esm mode resolutions don't include automatic extension lookup (without additional flags, at least) | ||
| // ./foo -> ./foo.ts | ||
| if (!(state.features & NodeResolutionFeatures.EsmMode)) { | ||
| // First, try adding an extension. An import of "foo" could be matched by a file "foo.ts", or "foo.js" by "foo.js.ts" | ||
| const resolvedByAddingExtension = tryAddingExtensions(candidate, extensions, "", onlyRecordFailures, state); | ||
| if (resolvedByAddingExtension) { | ||
| return resolvedByAddingExtension; | ||
| } | ||
| } | ||
|
|
||
| return loadModuleFromFileNoImplicitExtensions(extensions, candidate, onlyRecordFailures, state); |
There was a problem hiding this comment.
This is the extension adding/replacing order swap
| export default 0; | ||
|
|
||
| // @Filename: /a.ts | ||
| import b from "./b"; |
There was a problem hiding this comment.
This test asserted the incorrect behavior. It’s replaced by the two new tests.
jakebailey
approved these changes
Nov 10, 2022
| ImplementationFiles = TypeScript | JavaScript, | ||
| } | ||
|
|
||
| function formatExtensions(extensions: Extensions) { |
There was a problem hiding this comment.
FWIW you can get a pretty similar result with Debug.formatEnum. Post-modules that's something I want to yank out into a module for use outside of Debug.
There was a problem hiding this comment.
I didn’t use it mostly because it was in Debug and this is production code... I know it would work without any issue but it felt wrong.
There was a problem hiding this comment.
Yeah, I wouldn't have mentioned it but this seemed like it was just in tracing which straddles the line a little.
I think this might be a problem for monorepos where |
|
@typescript-bot test top100 |
|
Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at ba45a79. You can monitor the build here. Update: The results are in! |
|
@typescript-bot perf test faster |
|
Heya @andrewbranch, I've started to run the abridged perf test suite on this PR at ba45a79. You can monitor the build here. Update: The results are in! |
if this is breaking change anyway why not correct it for all algorithms.. Havent looked at changes yet and will do soon but does it simplify code even further or it makes it worse. Apart from being breaking change does it have any other reasoning not to break these two modes. Will it not do right thing. |
|
I intend for |
|
@andrewbranch Here they are:Comparison Report - main..51471
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@andrewbranch Here are the results of running the top-repos suite comparing Everything looks good! |
|
I really expected a couple monorepos to fail there. Interesting. |
|
Ah, the lack of failures could be due to low usage of |
|
@typescript-bot test top100 |
|
Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at 4014f30. You can monitor the build here. Update: The results are in! |
| extensions = [Extensions.TsOnly]; | ||
| if (compilerOptions.allowJs) extensions.push(Extensions.JavaScript); | ||
| if (compilerOptions.resolveJsonModule) extensions.push(Extensions.Json); | ||
| extensions = Extensions.ImplementationFiles; |
There was a problem hiding this comment.
this is the part where we are saying always looks for js and report error if allowJs is not on correct.
There was a problem hiding this comment.
This is actually only used by Find Source Definition, which always passes allowJs. But I removed the condition to be consistent with other resolution modes which always resolve JS, yes.
There was a problem hiding this comment.
And to be clear, normal resolution has resolved JS and reported errors afterwards for like 6 years; that’s not new here 😄
|
@typescript-bot perf test this |
|
Heya @andrewbranch, I've started to run the perf test suite on this PR at 4014f30. You can monitor the build here. Update: The results are in! |
|
@typescript-bot test top100 again, even though you haven’t finished the last one |
|
Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at 53f0a60. You can monitor the build here. Update: The results are in! |
|
@andrewbranch Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
@andrewbranch Here they are:
CompilerComparison Report - main..51471
System
Hosts
Scenarios
TSServerComparison Report - main..51471
System
Hosts
Scenarios
StartupComparison Report - main..51471
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
^ Yeah, that’s what I expected. I’ve reverted the node_modules searching algorithm back to two full passes, so I expect the second top100 run to come back clean. Updated the PR description:
|
|
@andrewbranch Here are the results of running the top-repos suite comparing Everything looks good! |
sheetalkamat
approved these changes
Nov 11, 2022
|
#51973 found an example of a break (a hypothetical break—the project uses "paths": {
"*": [
"node_modules/*",
"types/*"
]
}If we evaluate the resolution of a package name, But in the new algorithm, "paths": {
"*": ["types/*"]
}There’s no need to put |
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.
Fixes two forever-standing resolution bugs:
"./foo.js", we would look up"./foo.js.ts"at a higher priority than"./foo.ts". This has been fixed in every module resolution mode..tsor.d.tsfiles, and then another full pass looking for.jsfiles if the former returned nothing. This results in some priorities that are obviously wrong underallowJs, and sinceallowJsdoesn’t actually affect resolution results (only whether JS resolutions are errors after the fact), probably always wrong by extension. An example:import "./dir"should resolve todir.js, but previously resolved todir/index.tsin every module resolution algorithm. This has been fixed only innode16andnodenextin an attempt to limit breaking changes toclassicandnode. Those algorithms now try all valid extensions in priority order for each lookup location. The exception is that node_modules lookups still happen in two passes: one that searches the implementation package followed by the@typespackage looking for.ts/.d.tsfiles for eachnode_modulesfolder found up the directory spine, followed (if necessary) by another pass up the directory spine looking for.jsfiles in implementation packages. Making this change required a pretty extensive refactor of how the algorithms deal with extensions, and I think the result is much easier to reason about.For file system hits and watches, this should be a wash—depending on what files are where, a given resolution might have a few more lookups, a few less, or the same number but in a different order.
Like most bug fixes, this is technically a breaking change, but only very strange edge cases that are already misbehaving should be affected.