-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Fix ESM/CJS resolution cache collision in non-nodenext resolution modes #60910
base: main
Are you sure you want to change the base?
Conversation
@@ -2989,7 +2989,7 @@ function loadModuleFromNearestNodeModulesDirectoryTypesScope(moduleName: string, | |||
} | |||
|
|||
function loadModuleFromNearestNodeModulesDirectoryWorker(extensions: Extensions, moduleName: string, directory: string, state: ModuleResolutionState, typesScopeOnly: boolean, cache: ModuleResolutionCache | undefined, redirectedReference: ResolvedProjectReference | undefined): SearchResult<Resolved> { | |||
const mode = state.features === 0 ? undefined : state.features & NodeResolutionFeatures.EsmMode ? ModuleKind.ESNext : ModuleKind.CommonJS; | |||
const mode = state.features === 0 ? undefined : (state.features & NodeResolutionFeatures.EsmMode || state.conditions.includes("import")) ? ModuleKind.ESNext : ModuleKind.CommonJS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should possibly just plumb resolutionMode
through from the top, but this should work too. EsmMode
is never set outside of node16
–nodenext
; it’s only used to disable directory lookups and extension searching. conditions
is where we see the actual difference in different import modes in --moduleResolution bundler
.
@typescript-bot pack this |
Hey @andrewbranch, 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 |
@typescript-bot test it |
@andrewbranch Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
Hey @andrewbranch, the results of running the DT tests are ready. Everything looks the same! |
@andrewbranch Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Fixes #60243 for me, and more directly an incorrect resolution result shown in the conformance test. The crash at #60243 occurs when a single file contains a duplicated resolution stored under two different mode-aware cache keys. In the test case, the result for
import type pkg from "pkg" with { "resolution-mode": "require" }
is stored in the node_modules directory cache under1|pkg
. That result is returned for the"resolution-mode": "import"
resolution because themode
is recomputed incorrectly inloadModuleFromNearestNodeModulesDirectoryWorker
. This returns a wrong result, but it alsoresults in that resolution occurring twice in the per-file resolution, once under
1|pkg
and once under99|pkg
, so when we iterate over resolutions to stop watching them, we try to process the same resolution twice, causing the crash.