Skip to content

Commit b5f2ae7

Browse files
sheetalkamatsheetalkamat
committed
Auto type reference directive shouldnt lookup in node_modules
Fixes #37708 Bug 1 part of the issue
1 parent 51eea46 commit b5f2ae7

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
@@ -52,6 +52,7 @@ import {
5252
hasProperty,
5353
hasTrailingDirectorySeparator,
5454
hostGetCanonicalFileName,
55+
inferredTypesContainingFile,
5556
isArray,
5657
isExternalModuleNameRelative,
5758
isRootedDiskPath,
@@ -395,27 +396,19 @@ export function getEffectiveTypeRoots(options: CompilerOptions, host: GetEffecti
395396
}
396397

397398
if (currentDirectory !== undefined) {
398-
return getDefaultTypeRoots(currentDirectory, host);
399+
return getDefaultTypeRoots(currentDirectory);
399400
}
400401
}
401402

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

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

‎src/compiler/resolutionCache.ts

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

12051205
function createTypeRootsWatch(typeRootPath: Path, typeRoot: string): FileWatcher {
12061206
// Create new watch and recursive info
1207-
return resolutionHost.watchTypeRootsDirectory(typeRoot, fileOrDirectory => {
1208-
const fileOrDirectoryPath = resolutionHost.toPath(fileOrDirectory);
1209-
if (cachedDirectoryStructureHost) {
1210-
// Since the file existence changed, update the sourceFiles cache
1211-
cachedDirectoryStructureHost.addOrDeleteFileOrDirectory(fileOrDirectory, fileOrDirectoryPath);
1212-
}
1213-
1214-
// For now just recompile
1215-
// We could potentially store more data here about whether it was/would be really be used or not
1216-
// and with that determine to trigger compilation but for now this is enough
1217-
hasChangedAutomaticTypeDirectiveNames = true;
1218-
resolutionHost.onChangedAutomaticTypeDirectiveNames();
1207+
return canWatchTypeRootPath(typeRootPath) ?
1208+
resolutionHost.watchTypeRootsDirectory(typeRoot, fileOrDirectory => {
1209+
const fileOrDirectoryPath = resolutionHost.toPath(fileOrDirectory);
1210+
if (cachedDirectoryStructureHost) {
1211+
// Since the file existence changed, update the sourceFiles cache
1212+
cachedDirectoryStructureHost.addOrDeleteFileOrDirectory(fileOrDirectory, fileOrDirectoryPath);
1213+
}
12191214

1220-
// Since directory watchers invoked are flaky, the failed lookup location events might not be triggered
1221-
// So handle to failed lookup locations here as well to ensure we are invalidating resolutions
1222-
const dirPath = getDirectoryToWatchFailedLookupLocationFromTypeRoot(typeRoot, typeRootPath);
1223-
if (dirPath) {
1224-
scheduleInvalidateResolutionOfFailedLookupLocation(fileOrDirectoryPath, dirPath === fileOrDirectoryPath);
1225-
}
1226-
}, WatchDirectoryFlags.Recursive);
1215+
// For now just recompile
1216+
// We could potentially store more data here about whether it was/would be really be used or not
1217+
// and with that determine to trigger compilation but for now this is enough
1218+
hasChangedAutomaticTypeDirectiveNames = true;
1219+
resolutionHost.onChangedAutomaticTypeDirectiveNames();
1220+
1221+
// Since directory watchers invoked are flaky, the failed lookup location events might not be triggered
1222+
// So handle to failed lookup locations here as well to ensure we are invalidating resolutions
1223+
const dirPath = getDirectoryToWatchFailedLookupLocationFromTypeRoot(typeRoot, typeRootPath);
1224+
if (dirPath) {
1225+
scheduleInvalidateResolutionOfFailedLookupLocation(fileOrDirectoryPath, dirPath === fileOrDirectoryPath);
1226+
}
1227+
}, WatchDirectoryFlags.Recursive) :
1228+
noopFileWatcher;
12271229
}
12281230

12291231
/**
@@ -1241,7 +1243,7 @@ export function createResolutionCache(resolutionHost: ResolutionCacheHost, rootD
12411243

12421244
// we need to assume the directories exist to ensure that we can get all the type root directories that get included
12431245
// But filter directories that are at root level to say directory doesnt exist, so that we arent watching them
1244-
const typeRoots = getEffectiveTypeRoots(options, { directoryExists: directoryExistsForTypeRootWatch, getCurrentDirectory });
1246+
const typeRoots = getEffectiveTypeRoots(options, { getCurrentDirectory });
12451247
if (typeRoots) {
12461248
mutateMap(
12471249
typeRootsWatches,
@@ -1257,12 +1259,11 @@ export function createResolutionCache(resolutionHost: ResolutionCacheHost, rootD
12571259
}
12581260
}
12591261

1260-
/**
1261-
* Use this function to return if directory exists to get type roots to watch
1262-
* If we return directory exists then only the paths will be added to type roots
1263-
* Hence return true for all directories except root directories which are filtered from watching
1264-
*/
1265-
function directoryExistsForTypeRootWatch(nodeTypesDirectory: string) {
1262+
function canWatchTypeRootPath(nodeTypesDirectory: string) {
1263+
// If type roots is specified, watch that path
1264+
if (resolutionHost.getCompilationSettings().typeRoots) return true;
1265+
1266+
// Otherwise can watch directory only if we can watch the parent directory of node_modules/@types
12661267
const dir = getDirectoryPath(getDirectoryPath(nodeTypesDirectory));
12671268
const dirPath = resolutionHost.toPath(dir);
12681269
return dirPath === rootPath || canWatchDirectoryOrFile(dirPath);

‎src/compiler/types.ts

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

90509050
export interface GetEffectiveTypeRootsHost {
9051-
directoryExists?(directoryName: string): boolean;
90529051
getCurrentDirectory?(): string;
90539052
}
90549053

‎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
@@ -341,27 +341,32 @@ describe("unittests:: tsserver:: resolutionCache:: tsserverProjectSystem rename
341341
projectService.checkNumberOfProjects({ configuredProjects: 1 });
342342
});
343343

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

‎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)