Skip to content

Commit 40bb75c

Browse files
sheetalkamatsheetalkamat
committed
Auto type reference directive shouldnt lookup in node_modules
Fixes #37708 Bug 1 part of the issue
1 parent 4ea628e commit 40bb75c

File tree

31 files changed

+527
-294
lines changed

31 files changed

+527
-294
lines changed

‎src/compiler/diagnosticMessages.json

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5154,6 +5154,10 @@
51545154
"category": "Message",
51555155
"code": 6261
51565156
},
5157+
"Resolving type reference directive for program that specifies custom typeRoots, skipping lookup in 'node_modules' folder.": {
5158+
"category": "Message",
5159+
"code": 6262
5160+
},
51575161

51585162
"Directory '{0}' has no containing package.json scope. Imports will not resolve.": {
51595163
"category": "Message",

‎src/compiler/moduleNameResolver.ts

Lines changed: 19 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ import {
5454
hasProperty,
5555
hasTrailingDirectorySeparator,
5656
hostGetCanonicalFileName,
57+
inferredTypesContainingFile,
5758
isArray,
5859
isExternalModuleNameRelative,
5960
isRootedDiskPath,
@@ -396,27 +397,19 @@ export function getEffectiveTypeRoots(options: CompilerOptions, host: GetEffecti
396397
}
397398

398399
if (currentDirectory !== undefined) {
399-
return getDefaultTypeRoots(currentDirectory, host);
400+
return getDefaultTypeRoots(currentDirectory);
400401
}
401402
}
402403

403404
/**
404405
* Returns the path to every node_modules/@types directory from some ancestor directory.
405406
* Returns undefined if there are none.
406407
*/
407-
function getDefaultTypeRoots(currentDirectory: string, host: { directoryExists?: (directoryName: string) => boolean }): string[] | undefined {
408-
if (!host.directoryExists) {
409-
return [combinePaths(currentDirectory, nodeModulesAtTypes)];
410-
// And if it doesn't exist, tough.
411-
}
412-
408+
function getDefaultTypeRoots(currentDirectory: string): string[] | undefined {
413409
let typeRoots: string[] | undefined;
414410
forEachAncestorDirectory(normalizePath(currentDirectory), directory => {
415411
const atTypes = combinePaths(directory, nodeModulesAtTypes);
416-
if (host.directoryExists!(atTypes)) {
417-
(typeRoots || (typeRoots = [])).push(atTypes);
418-
}
419-
return undefined;
412+
(typeRoots ??= []).push(atTypes);
420413
});
421414
return typeRoots;
422415
}
@@ -576,20 +569,24 @@ export function resolveTypeReferenceDirective(typeReferenceDirectiveName: string
576569

577570
function secondaryLookup(): PathAndPackageId | undefined {
578571
const initialLocationForSecondaryLookup = containingFile && getDirectoryPath(containingFile);
579-
580572
if (initialLocationForSecondaryLookup !== undefined) {
581-
// check secondary locations
582-
if (traceEnabled) {
583-
trace(host, Diagnostics.Looking_up_in_node_modules_folder_initial_location_0, initialLocationForSecondaryLookup);
584-
}
585573
let result: Resolved | undefined;
586-
if (!isExternalModuleNameRelative(typeReferenceDirectiveName)) {
587-
const searchResult = loadModuleFromNearestNodeModulesDirectory(Extensions.Declaration, typeReferenceDirectiveName, initialLocationForSecondaryLookup, moduleResolutionState, /*cache*/ undefined, /*redirectedReference*/ undefined);
588-
result = searchResult && searchResult.value;
574+
if (!options.typeRoots || !endsWith(containingFile!, inferredTypesContainingFile)) {
575+
// check secondary locations
576+
if (traceEnabled) {
577+
trace(host, Diagnostics.Looking_up_in_node_modules_folder_initial_location_0, initialLocationForSecondaryLookup);
578+
}
579+
if (!isExternalModuleNameRelative(typeReferenceDirectiveName)) {
580+
const searchResult = loadModuleFromNearestNodeModulesDirectory(Extensions.Declaration, typeReferenceDirectiveName, initialLocationForSecondaryLookup, moduleResolutionState, /*cache*/ undefined, /*redirectedReference*/ undefined);
581+
result = searchResult && searchResult.value;
582+
}
583+
else {
584+
const { path: candidate } = normalizePathForCJSResolution(initialLocationForSecondaryLookup, typeReferenceDirectiveName);
585+
result = nodeLoadModuleByRelativeName(Extensions.Declaration, candidate, /*onlyRecordFailures*/ false, moduleResolutionState, /*considerPackageJson*/ true);
586+
}
589587
}
590-
else {
591-
const { path: candidate } = normalizePathForCJSResolution(initialLocationForSecondaryLookup, typeReferenceDirectiveName);
592-
result = nodeLoadModuleByRelativeName(Extensions.Declaration, candidate, /*onlyRecordFailures*/ false, moduleResolutionState, /*considerPackageJson*/ true);
588+
else if (traceEnabled) {
589+
trace(host, Diagnostics.Resolving_type_reference_directive_for_program_that_specifies_custom_typeRoots_skipping_lookup_in_node_modules_folder);
593590
}
594591
return resolvedTypeScriptOnly(result);
595592
}

‎src/compiler/resolutionCache.ts

Lines changed: 27 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1157,26 +1157,28 @@ export function createResolutionCache(resolutionHost: ResolutionCacheHost, rootD
11571157

11581158
function createTypeRootsWatch(typeRootPath: Path, typeRoot: string): FileWatcher {
11591159
// Create new watch and recursive info
1160-
return resolutionHost.watchTypeRootsDirectory(typeRoot, fileOrDirectory => {
1161-
const fileOrDirectoryPath = resolutionHost.toPath(fileOrDirectory);
1162-
if (cachedDirectoryStructureHost) {
1163-
// Since the file existence changed, update the sourceFiles cache
1164-
cachedDirectoryStructureHost.addOrDeleteFileOrDirectory(fileOrDirectory, fileOrDirectoryPath);
1165-
}
1166-
1167-
// For now just recompile
1168-
// We could potentially store more data here about whether it was/would be really be used or not
1169-
// and with that determine to trigger compilation but for now this is enough
1170-
hasChangedAutomaticTypeDirectiveNames = true;
1171-
resolutionHost.onChangedAutomaticTypeDirectiveNames();
1160+
return canWatchTypeRootPath(typeRootPath) ?
1161+
resolutionHost.watchTypeRootsDirectory(typeRoot, fileOrDirectory => {
1162+
const fileOrDirectoryPath = resolutionHost.toPath(fileOrDirectory);
1163+
if (cachedDirectoryStructureHost) {
1164+
// Since the file existence changed, update the sourceFiles cache
1165+
cachedDirectoryStructureHost.addOrDeleteFileOrDirectory(fileOrDirectory, fileOrDirectoryPath);
1166+
}
11721167

1173-
// Since directory watchers invoked are flaky, the failed lookup location events might not be triggered
1174-
// So handle to failed lookup locations here as well to ensure we are invalidating resolutions
1175-
const dirPath = getDirectoryToWatchFailedLookupLocationFromTypeRoot(typeRoot, typeRootPath);
1176-
if (dirPath) {
1177-
scheduleInvalidateResolutionOfFailedLookupLocation(fileOrDirectoryPath, dirPath === fileOrDirectoryPath);
1178-
}
1179-
}, WatchDirectoryFlags.Recursive);
1168+
// For now just recompile
1169+
// We could potentially store more data here about whether it was/would be really be used or not
1170+
// and with that determine to trigger compilation but for now this is enough
1171+
hasChangedAutomaticTypeDirectiveNames = true;
1172+
resolutionHost.onChangedAutomaticTypeDirectiveNames();
1173+
1174+
// Since directory watchers invoked are flaky, the failed lookup location events might not be triggered
1175+
// So handle to failed lookup locations here as well to ensure we are invalidating resolutions
1176+
const dirPath = getDirectoryToWatchFailedLookupLocationFromTypeRoot(typeRoot, typeRootPath);
1177+
if (dirPath) {
1178+
scheduleInvalidateResolutionOfFailedLookupLocation(fileOrDirectoryPath, dirPath === fileOrDirectoryPath);
1179+
}
1180+
}, WatchDirectoryFlags.Recursive) :
1181+
noopFileWatcher;
11801182
}
11811183

11821184
/**
@@ -1194,7 +1196,7 @@ export function createResolutionCache(resolutionHost: ResolutionCacheHost, rootD
11941196

11951197
// we need to assume the directories exist to ensure that we can get all the type root directories that get included
11961198
// But filter directories that are at root level to say directory doesnt exist, so that we arent watching them
1197-
const typeRoots = getEffectiveTypeRoots(options, { directoryExists: directoryExistsForTypeRootWatch, getCurrentDirectory });
1199+
const typeRoots = getEffectiveTypeRoots(options, { getCurrentDirectory });
11981200
if (typeRoots) {
11991201
mutateMap(
12001202
typeRootsWatches,
@@ -1210,12 +1212,11 @@ export function createResolutionCache(resolutionHost: ResolutionCacheHost, rootD
12101212
}
12111213
}
12121214

1213-
/**
1214-
* Use this function to return if directory exists to get type roots to watch
1215-
* If we return directory exists then only the paths will be added to type roots
1216-
* Hence return true for all directories except root directories which are filtered from watching
1217-
*/
1218-
function directoryExistsForTypeRootWatch(nodeTypesDirectory: string) {
1215+
function canWatchTypeRootPath(nodeTypesDirectory: string) {
1216+
// If type roots is specified, watch that path
1217+
if (resolutionHost.getCompilationSettings().typeRoots) return true;
1218+
1219+
// Otherwise can watch directory only if we can watch the parent directory of node_modules/@types
12191220
const dir = getDirectoryPath(getDirectoryPath(nodeTypesDirectory));
12201221
const dirPath = resolutionHost.toPath(dir);
12211222
return dirPath === rootPath || canWatchDirectoryOrFile(dirPath);

‎src/compiler/types.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9049,7 +9049,6 @@ export interface EmitTextWriter extends SymbolWriter {
90499049
}
90509050

90519051
export interface GetEffectiveTypeRootsHost {
9052-
directoryExists?(directoryName: string): boolean;
90539052
getCurrentDirectory?(): string;
90549053
}
90559054

‎src/server/project.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2825,7 +2825,7 @@ export class ConfiguredProject extends Project {
28252825
}
28262826

28272827
getEffectiveTypeRoots() {
2828-
return getEffectiveTypeRoots(this.getCompilationSettings(), this.directoryStructureHost) || [];
2828+
return getEffectiveTypeRoots(this.getCompilationSettings(), this) || [];
28292829
}
28302830

28312831
/** @internal */

‎src/testRunner/unittests/tscWatch/programUpdates.ts

Lines changed: 31 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -937,29 +937,37 @@ declare const eval: any`
937937
]
938938
});
939939

940-
verifyTscWatch({
941-
scenario,
942-
subScenario: "types should load from config file path if config exists",
943-
commandLineArgs: ["-w", "-p", configFilePath],
944-
sys: () => {
945-
const f1 = {
946-
path: "/a/b/app.ts",
947-
content: "let x = 1"
948-
};
949-
const config = {
950-
path: configFilePath,
951-
content: JSON.stringify({ compilerOptions: { types: ["node"], typeRoots: [] } })
952-
};
953-
const node = {
954-
path: "/a/b/node_modules/@types/node/index.d.ts",
955-
content: "declare var process: any"
956-
};
957-
const cwd = {
958-
path: "/a/c"
959-
};
960-
return createWatchedSystem([f1, config, node, cwd, libFile], { currentDirectory: cwd.path });
961-
},
962-
changes: ts.emptyArray
940+
describe("types from config file", () => {
941+
function verifyTypesLoad(includeTypeRoots: boolean) {
942+
verifyTscWatch({
943+
scenario,
944+
subScenario: includeTypeRoots ?
945+
"types should not load from config file path if config exists but does not specifies typeRoots" :
946+
"types should load from config file path if config exists",
947+
commandLineArgs: ["-w", "-p", configFilePath],
948+
sys: () => {
949+
const f1 = {
950+
path: "/a/b/app.ts",
951+
content: "let x = 1"
952+
};
953+
const config = {
954+
path: configFilePath,
955+
content: JSON.stringify({ compilerOptions: { types: ["node"], typeRoots: includeTypeRoots ? [] : undefined } })
956+
};
957+
const node = {
958+
path: "/a/b/node_modules/@types/node/index.d.ts",
959+
content: "declare var process: any"
960+
};
961+
const cwd = {
962+
path: "/a/c"
963+
};
964+
return createWatchedSystem([f1, config, node, cwd, libFile], { currentDirectory: cwd.path });
965+
},
966+
changes: ts.emptyArray
967+
});
968+
}
969+
verifyTypesLoad(/*includeTypeRoots*/ false);
970+
verifyTypesLoad(/*includeTypeRoots*/ true);
963971
});
964972

965973
verifyTscWatch({

‎src/testRunner/unittests/tsserver/resolutionCache.ts

Lines changed: 26 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -345,27 +345,32 @@ describe("unittests:: tsserver:: resolutionCache:: tsserverProjectSystem rename
345345
projectService.checkNumberOfProjects({ configuredProjects: 1 });
346346
});
347347

348-
it("types should load from config file path if config exists", () => {
349-
const f1 = {
350-
path: "/a/b/app.ts",
351-
content: "let x = 1"
352-
};
353-
const config = {
354-
path: "/a/b/tsconfig.json",
355-
content: JSON.stringify({ compilerOptions: { types: ["node"], typeRoots: [] } })
356-
};
357-
const node = {
358-
path: "/a/b/node_modules/@types/node/index.d.ts",
359-
content: "declare var process: any"
360-
};
361-
const cwd = {
362-
path: "/a/c"
363-
};
364-
const host = createServerHost([f1, config, node, cwd], { currentDirectory: cwd.path });
365-
const projectService = createProjectService(host);
366-
projectService.openClientFile(f1.path);
367-
projectService.checkNumberOfProjects({ configuredProjects: 1 });
368-
checkProjectActualFiles(configuredProjectAt(projectService, 0), [f1.path, node.path, config.path]);
348+
describe("types from config file", () => {
349+
function verifyTypesLoad(subScenario: string, includeTypeRoots: boolean) {
350+
it(subScenario, () => {
351+
const f1 = {
352+
path: "/a/b/app.ts",
353+
content: "let x = 1"
354+
};
355+
const config = {
356+
path: "/a/b/tsconfig.json",
357+
content: JSON.stringify({ compilerOptions: { types: ["node"], typeRoots: includeTypeRoots ? [] : undefined } })
358+
};
359+
const node = {
360+
path: "/a/b/node_modules/@types/node/index.d.ts",
361+
content: "declare var process: any"
362+
};
363+
const cwd = {
364+
path: "/a/c"
365+
};
366+
const host = createServerHost([f1, config, node, cwd], { currentDirectory: cwd.path });
367+
const projectService = createProjectService(host, { logger: createLoggerWithInMemoryLogs(host) });
368+
projectService.openClientFile(f1.path);
369+
baselineTsserverLogs("resolutionCache", subScenario, projectService);
370+
});
371+
}
372+
verifyTypesLoad("types should load from config file path if config exists", /*includeTypeRoots*/ false);
373+
verifyTypesLoad("types should not load from config file path if config exists but does not specifies typeRoots", /*includeTypeRoots*/ true);
369374
});
370375
});
371376

‎tests/baselines/reference/api/tsserverlibrary.d.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8258,7 +8258,6 @@ declare namespace ts {
82588258
noEmitHelpers?: boolean;
82598259
}
82608260
interface GetEffectiveTypeRootsHost {
8261-
directoryExists?(directoryName: string): boolean;
82628261
getCurrentDirectory?(): string;
82638262
}
82648263
interface TextSpan {

‎tests/baselines/reference/api/typescript.d.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4322,7 +4322,6 @@ declare namespace ts {
43224322
noEmitHelpers?: boolean;
43234323
}
43244324
interface GetEffectiveTypeRootsHost {
4325-
directoryExists?(directoryName: string): boolean;
43264325
getCurrentDirectory?(): string;
43274326
}
43284327
interface TextSpan {

‎tests/baselines/reference/library-reference-11.trace.json

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
[
2-
"======== Resolving type reference directive 'jquery', containing file '/a/b/consumer.ts', root directory not set. ========",
3-
"Root directory cannot be determined, skipping primary search paths.",
2+
"======== Resolving type reference directive 'jquery', containing file '/a/b/consumer.ts', root directory '/node_modules/@types'. ========",
3+
"Resolving with primary search path '/node_modules/@types'.",
4+
"Directory '/node_modules/@types' does not exist, skipping all lookups in it.",
45
"Looking up in 'node_modules' folder, initial location '/a/b'.",
56
"Directory '/a/b/node_modules' does not exist, skipping all lookups in it.",
67
"Found 'package.json' at '/a/node_modules/jquery/package.json'.",

‎tests/baselines/reference/library-reference-12.trace.json

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
[
2-
"======== Resolving type reference directive 'jquery', containing file '/a/b/consumer.ts', root directory not set. ========",
3-
"Root directory cannot be determined, skipping primary search paths.",
2+
"======== Resolving type reference directive 'jquery', containing file '/a/b/consumer.ts', root directory '/node_modules/@types'. ========",
3+
"Resolving with primary search path '/node_modules/@types'.",
4+
"Directory '/node_modules/@types' does not exist, skipping all lookups in it.",
45
"Looking up in 'node_modules' folder, initial location '/a/b'.",
56
"Directory '/a/b/node_modules' does not exist, skipping all lookups in it.",
67
"Found 'package.json' at '/a/node_modules/jquery/package.json'.",

‎tests/baselines/reference/library-reference-3.trace.json

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
[
2-
"======== Resolving type reference directive 'jquery', containing file '/src/consumer.ts', root directory not set. ========",
3-
"Root directory cannot be determined, skipping primary search paths.",
2+
"======== Resolving type reference directive 'jquery', containing file '/src/consumer.ts', root directory '/src/node_modules/@types,/node_modules/@types'. ========",
3+
"Resolving with primary search path '/src/node_modules/@types, /node_modules/@types'.",
4+
"Directory '/src/node_modules/@types' does not exist, skipping all lookups in it.",
5+
"Directory '/node_modules/@types' does not exist, skipping all lookups in it.",
46
"Looking up in 'node_modules' folder, initial location '/src'.",
57
"File '/src/node_modules/jquery/package.json' does not exist.",
68
"File '/src/node_modules/jquery.d.ts' does not exist.",

‎tests/baselines/reference/library-reference-7.trace.json

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
[
2-
"======== Resolving type reference directive 'jquery', containing file '/src/consumer.ts', root directory not set. ========",
3-
"Root directory cannot be determined, skipping primary search paths.",
2+
"======== Resolving type reference directive 'jquery', containing file '/src/consumer.ts', root directory '/node_modules/@types'. ========",
3+
"Resolving with primary search path '/node_modules/@types'.",
4+
"Directory '/node_modules/@types' does not exist, skipping all lookups in it.",
45
"Looking up in 'node_modules' folder, initial location '/src'.",
56
"File '/src/node_modules/jquery/package.json' does not exist.",
67
"File '/src/node_modules/jquery.d.ts' does not exist.",

‎tests/baselines/reference/moduleResolutionWithSymlinks_preserveSymlinks.trace.json

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
[
2-
"======== Resolving type reference directive 'linked', containing file '/app/app.ts', root directory not set. ========",
3-
"Root directory cannot be determined, skipping primary search paths.",
2+
"======== Resolving type reference directive 'linked', containing file '/app/app.ts', root directory 'node_modules/@types,/node_modules/@types'. ========",
3+
"Resolving with primary search path 'node_modules/@types, /node_modules/@types'.",
4+
"Directory 'node_modules/@types' does not exist, skipping all lookups in it.",
5+
"Directory '/node_modules/@types' does not exist, skipping all lookups in it.",
46
"Looking up in 'node_modules' folder, initial location '/app'.",
57
"File '/app/node_modules/linked/package.json' does not exist.",
68
"File '/app/node_modules/linked.d.ts' does not exist.",

0 commit comments

Comments
 (0)