-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Reuse Module Resolutions Within Project References During Initial Program Creation #40964
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1107,7 +1107,7 @@ namespace ts { | |
} | ||
|
||
function resolveModuleNamesReusingOldState(moduleNames: string[], file: SourceFile): readonly ResolvedModuleFull[] { | ||
if (structuralIsReused === StructureIsReused.Not && !file.ambientModuleNames.length) { | ||
if (!doesStructuralPermitResolutionsReuse() && !file.ambientModuleNames.length) { | ||
// If the old program state does not permit reusing resolutions and `file` does not contain locally defined ambient modules, | ||
// the best we can do is fallback to the default logic. | ||
return resolveModuleNamesWorker(moduleNames, file, /*reusedNames*/ undefined); | ||
|
@@ -1218,6 +1218,31 @@ namespace ts { | |
|
||
return result; | ||
|
||
function doesStructuralPermitResolutionsReuse(): boolean { | ||
switch (structuralIsReused) { | ||
case StructureIsReused.Not: return false; | ||
case StructureIsReused.SafeProjectReferenceModules: | ||
return file.resolvedModules !== undefined && | ||
isSourceOfProjectReferenceRedirect(file.originalFileName); | ||
case StructureIsReused.SafeModules: return true; | ||
case StructureIsReused.Completely: return true; | ||
|
||
// It's possible for resolveModuleNamesReusingOldState to be called by | ||
// tryReuseStructureFromOldProgram before structuralIsReused has been defined. | ||
// In this case, the caller (tryReuseStructureFromOldProgram) expects this | ||
// function to reuse resolutions for its checks. Returning true to accommodate | ||
// for that specific scenario. | ||
// | ||
// TODO: Clean this because the above scenario is a bit unexpected. This case | ||
// should ideally return false or throw. | ||
case undefined: return true; | ||
|
||
// The above cases should exhaustively cover all branches. Defaulting to false | ||
// since that's safer in unexpected situations. | ||
default: return false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't an exhaustive switch statement because |
||
} | ||
} | ||
|
||
// If we change our policy of rechecking failed lookups on each program create, | ||
// we should adjust the value returned here. | ||
function moduleNameResolvesToAmbientModuleInNonModifiedFile(moduleName: string): boolean { | ||
|
@@ -1271,7 +1296,10 @@ namespace ts { | |
|
||
function tryReuseStructureFromOldProgram(): StructureIsReused { | ||
if (!oldProgram) { | ||
return StructureIsReused.Not; | ||
// During initial program creation, root files may import files from project | ||
// references that were previously loaded. Those resolutions are safe to reuse | ||
// since another program instance kept them up to date. | ||
return StructureIsReused.SafeProjectReferenceModules; | ||
} | ||
|
||
// check properties that can affect structure of the program or module resolution strategy | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -793,6 +793,92 @@ namespace ts { | |
} | ||
}); | ||
|
||
it("can reuse module resolutions within project references", () => { | ||
const commonOptions = { | ||
composite: true, | ||
declaration: true, | ||
target: ScriptTarget.ES2015, | ||
traceResolution: true, | ||
moduleResolution: ModuleResolutionKind.Classic, | ||
}; | ||
const tsconfigA = { | ||
compilerOptions: commonOptions, | ||
include: ["src"] | ||
}; | ||
const tsconfigB = { | ||
compilerOptions: { | ||
...commonOptions, | ||
paths: { | ||
a: ["/a/src/index.ts"] | ||
} | ||
}, | ||
projectReferences: [ | ||
{ path: "/a" } | ||
] | ||
}; | ||
|
||
const files = [ | ||
{ name: "/a/src/x.ts", text: SourceText.New("", "export const x = 1;", "") }, | ||
{ name: "/a/src/index.ts", text: SourceText.New("", "export { x } from './x';", "") }, | ||
{ name: "/a/tsconfig.json", text: SourceText.New("", "", JSON.stringify(tsconfigA)) }, | ||
{ name: "/b/src/b.ts", text: SourceText.New("", "import { x } from 'a';", "") }, | ||
]; | ||
const rootNamesA = ["/a/src/index.ts", "/a/src/x.ts"]; | ||
const rootNamesB = ["/b/src/b.ts"]; | ||
|
||
const host = createTestCompilerHost(files, commonOptions.target); | ||
|
||
// Instead of hard-coding file system entries for this test, we could also write a function that more | ||
// generally transforms a list of files into a tree of FileSystemEntries. | ||
function getFileSystemEntries(path: string) { | ||
const mapPathToFileSystemEntries: { [path: string]: FileSystemEntries | undefined } = { | ||
"/a": { files: [], directories: ["src"] }, | ||
"/a/src": { files: ["index.ts", "x.ts"], directories: [] } | ||
}; | ||
|
||
const entries = mapPathToFileSystemEntries[path]; | ||
if (!entries) { | ||
throw new Error(`Unexpected path "${path}" requested from readDirectory. Test is broken.`); | ||
} | ||
return entries; | ||
} | ||
Comment on lines
+831
to
+844
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This part is a bit ugly. I'm welcome for suggestions. |
||
|
||
host.readDirectory = (rootDir, extensions, excludes, includes, depth) => | ||
matchFiles( | ||
rootDir, | ||
extensions, | ||
excludes, | ||
includes, | ||
/*useCaseSensitiveFileNames*/ true, | ||
/*currentDirectory*/ "/", | ||
depth, | ||
getFileSystemEntries, | ||
/*realpath*/ path => path); | ||
|
||
createProgram(rootNamesA, tsconfigA.compilerOptions, host); | ||
createProgram({ | ||
rootNames: rootNamesB, | ||
options: tsconfigB.compilerOptions, | ||
host, | ||
projectReferences: tsconfigB.projectReferences | ||
}); | ||
|
||
// Resolution should not be performed for "./x" from "/a/src/index.ts" multiple times. | ||
assert.deepEqual(host.getTrace(), [ | ||
"======== Resolving module './x' from '/a/src/index.ts'. ========", | ||
"Explicitly specified module resolution kind: 'Classic'.", | ||
"File '/a/src/x.ts' exist - use it as a name resolution result.", | ||
"======== Module name './x' was successfully resolved to '/a/src/x.ts'. ========", | ||
"======== Resolving module 'a' from '/b/src/b.ts'. ========", | ||
"Explicitly specified module resolution kind: 'Classic'.", | ||
"'paths' option is specified, looking for a pattern to match module name 'a'.", | ||
"Module name 'a', matched pattern 'a'.", | ||
"Trying substitution '/a/src/index.ts', candidate module location: '/a/src/index.ts'.", | ||
"File '/a/src/index.ts' exist - use it as a name resolution result.", | ||
"======== Module name 'a' was successfully resolved to '/a/src/index.ts'. ========", | ||
], "should reuse resolution to /a/src/x.ts"); | ||
}); | ||
|
||
describe("redirects", () => { | ||
const axIndex = "/node_modules/a/node_modules/x/index.d.ts"; | ||
const axPackage = "/node_modules/a/node_modules/x/package.json"; | ||
|
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.
I think I saw a rule that
TODO
lines need an associated issue, but I'll wait for someone to confirm that this comment is correct before I create that.