Auto type reference directive shouldnt lookup in node_modules

sheetalkamat · sheetalkamat · commit e64e70f6a7b9 · 2020-07-22T15:37:09.000-07:00
Fixes #37708 Bug 1 part of the issue
diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json
@@ -4473,6 +4473,10 @@
         "category": "Message",
         "code": 6235
     },
+    "Resolving type reference directive for program with typeRoots, skipping lookup in 'node_modules' folder.": {
+        "category": "Message",
+        "code": 6236
+    },
 
     "Projects to reference": {
         "category": "Message",
diff --git a/src/compiler/moduleNameResolver.ts b/src/compiler/moduleNameResolver.ts
@@ -250,27 +250,19 @@ namespace ts {
         }
 
         if (currentDirectory !== undefined) {
-            return getDefaultTypeRoots(currentDirectory, host);
+            return getDefaultTypeRoots(currentDirectory);
         }
     }
 
     /**
      * Returns the path to every node_modules/@types directory from some ancestor directory.
      * Returns undefined if there are none.
      */
-    function getDefaultTypeRoots(currentDirectory: string, host: { directoryExists?: (directoryName: string) => boolean }): string[] | undefined {
-        if (!host.directoryExists) {
-            return [combinePaths(currentDirectory, nodeModulesAtTypes)];
-            // And if it doesn't exist, tough.
-        }
-
+    function getDefaultTypeRoots(currentDirectory: string): string[] | undefined {
         let typeRoots: string[] | undefined;
         forEachAncestorDirectory(normalizePath(currentDirectory), directory => {
             const atTypes = combinePaths(directory, nodeModulesAtTypes);
-            if (host.directoryExists!(atTypes)) {
-                (typeRoots || (typeRoots = [])).push(atTypes);
-            }
-            return undefined;
+            (typeRoots || (typeRoots = [])).push(atTypes);
         });
         return typeRoots;
     }
@@ -363,20 +355,25 @@ namespace ts {
 
         function secondaryLookup(): PathAndPackageId | undefined {
             const initialLocationForSecondaryLookup = containingFile && getDirectoryPath(containingFile);
-
             if (initialLocationForSecondaryLookup !== undefined) {
-                // check secondary locations
-                if (traceEnabled) {
-                    trace(host, Diagnostics.Looking_up_in_node_modules_folder_initial_location_0, initialLocationForSecondaryLookup);
-                }
+                // Do not resolve auto types from secondary location
                 let result: Resolved | undefined;
-                if (!isExternalModuleNameRelative(typeReferenceDirectiveName)) {
-                    const searchResult = loadModuleFromNearestNodeModulesDirectory(Extensions.DtsOnly, typeReferenceDirectiveName, initialLocationForSecondaryLookup, moduleResolutionState, /*cache*/ undefined, /*redirectedReference*/ undefined);
-                    result = searchResult && searchResult.value;
+                if (!options.typeRoots || !endsWith(containingFile!, inferredTypesContainingFile)) {
+                    // check secondary locations
+                    if (traceEnabled) {
+                        trace(host, Diagnostics.Looking_up_in_node_modules_folder_initial_location_0, initialLocationForSecondaryLookup);
+                    }
+                    if (!isExternalModuleNameRelative(typeReferenceDirectiveName)) {
+                        const searchResult = loadModuleFromNearestNodeModulesDirectory(Extensions.DtsOnly, typeReferenceDirectiveName, initialLocationForSecondaryLookup, moduleResolutionState, /*cache*/ undefined, /*redirectedReference*/ undefined);
+                        result = searchResult && searchResult.value;
+                    }
+                    else {
+                        const { path: candidate } = normalizePathAndParts(combinePaths(initialLocationForSecondaryLookup, typeReferenceDirectiveName));
+                        result = nodeLoadModuleByRelativeName(Extensions.DtsOnly, candidate, /*onlyRecordFailures*/ false, moduleResolutionState, /*considerPackageJson*/ true);
+                    }
                 }
-                else {
-                    const { path: candidate } = normalizePathAndParts(combinePaths(initialLocationForSecondaryLookup, typeReferenceDirectiveName));
-                    result = nodeLoadModuleByRelativeName(Extensions.DtsOnly, candidate, /*onlyRecordFailures*/ false, moduleResolutionState, /*considerPackageJson*/ true);
+                else if (traceEnabled) {
+                    trace(host, Diagnostics.Resolving_type_reference_directive_for_program_with_typeRoots_skipping_lookup_in_node_modules_folder);
                 }
                 const resolvedFile = resolvedTypeScriptOnly(result);
                 if (!resolvedFile && traceEnabled) {
diff --git a/src/compiler/resolutionCache.ts b/src/compiler/resolutionCache.ts
@@ -869,26 +869,28 @@ namespace ts {
 
         function createTypeRootsWatch(typeRootPath: Path, typeRoot: string): FileWatcher {
             // Create new watch and recursive info
-            return resolutionHost.watchTypeRootsDirectory(typeRoot, fileOrDirectory => {
-                const fileOrDirectoryPath = resolutionHost.toPath(fileOrDirectory);
-                if (cachedDirectoryStructureHost) {
-                    // Since the file existence changed, update the sourceFiles cache
-                    cachedDirectoryStructureHost.addOrDeleteFileOrDirectory(fileOrDirectory, fileOrDirectoryPath);
-                }
-
-                // For now just recompile
-                // We could potentially store more data here about whether it was/would be really be used or not
-                // and with that determine to trigger compilation but for now this is enough
-                hasChangedAutomaticTypeDirectiveNames = true;
-                resolutionHost.onChangedAutomaticTypeDirectiveNames();
+            return canWatchTypeRootPath(typeRootPath) ?
+                resolutionHost.watchTypeRootsDirectory(typeRoot, fileOrDirectory => {
+                    const fileOrDirectoryPath = resolutionHost.toPath(fileOrDirectory);
+                    if (cachedDirectoryStructureHost) {
+                        // Since the file existence changed, update the sourceFiles cache
+                        cachedDirectoryStructureHost.addOrDeleteFileOrDirectory(fileOrDirectory, fileOrDirectoryPath);
+                    }
 
-                // Since directory watchers invoked are flaky, the failed lookup location events might not be triggered
-                // So handle to failed lookup locations here as well to ensure we are invalidating resolutions
-                const dirPath = getDirectoryToWatchFailedLookupLocationFromTypeRoot(typeRoot, typeRootPath);
-                if (dirPath) {
-                    scheduleInvalidateResolutionOfFailedLookupLocation(fileOrDirectoryPath, dirPath === fileOrDirectoryPath);
-                }
-            }, WatchDirectoryFlags.Recursive);
+                    // For now just recompile
+                    // We could potentially store more data here about whether it was/would be really be used or not
+                    // and with that determine to trigger compilation but for now this is enough
+                    hasChangedAutomaticTypeDirectiveNames = true;
+                    resolutionHost.onChangedAutomaticTypeDirectiveNames();
+
+                    // Since directory watchers invoked are flaky, the failed lookup location events might not be triggered
+                    // So handle to failed lookup locations here as well to ensure we are invalidating resolutions
+                    const dirPath = getDirectoryToWatchFailedLookupLocationFromTypeRoot(typeRoot, typeRootPath);
+                    if (dirPath) {
+                        scheduleInvalidateResolutionOfFailedLookupLocation(fileOrDirectoryPath, dirPath === fileOrDirectoryPath);
+                    }
+                }, WatchDirectoryFlags.Recursive) :
+                noopFileWatcher;
         }
 
         /**
@@ -906,7 +908,7 @@ namespace ts {
 
             // we need to assume the directories exist to ensure that we can get all the type root directories that get included
             // But filter directories that are at root level to say directory doesnt exist, so that we arent watching them
-            const typeRoots = getEffectiveTypeRoots(options, { directoryExists: directoryExistsForTypeRootWatch, getCurrentDirectory });
+            const typeRoots = getEffectiveTypeRoots(options, { getCurrentDirectory });
             if (typeRoots) {
                 mutateMap(
                     typeRootsWatches,
@@ -922,12 +924,11 @@ namespace ts {
             }
         }
 
-        /**
-         * Use this function to return if directory exists to get type roots to watch
-         * If we return directory exists then only the paths will be added to type roots
-         * Hence return true for all directories except root directories which are filtered from watching
-         */
-        function directoryExistsForTypeRootWatch(nodeTypesDirectory: string) {
+        function canWatchTypeRootPath(nodeTypesDirectory: string) {
+            // If type roots is specified, watch that path
+            if (resolutionHost.getCompilationSettings().typeRoots) return true;
+
+            // Otherwise can watch directory only if we can watch the parent directory of node_modules/@types
             const dir = getDirectoryPath(getDirectoryPath(nodeTypesDirectory));
             const dirPath = resolutionHost.toPath(dir);
             return dirPath === rootPath || canWatchDirectory(dirPath);
diff --git a/src/compiler/types.ts b/src/compiler/types.ts
@@ -7748,7 +7748,6 @@ namespace ts {
     }
 
     export interface GetEffectiveTypeRootsHost {
-        directoryExists?(directoryName: string): boolean;
         getCurrentDirectory?(): string;
     }
 
diff --git a/src/server/project.ts b/src/server/project.ts
@@ -2278,10 +2278,6 @@ namespace ts.server {
             return !!this.externalProjectRefCount;
         }
 
-        getEffectiveTypeRoots() {
-            return getEffectiveTypeRoots(this.getCompilationSettings(), this.directoryStructureHost) || [];
-        }
-
         /*@internal*/
         updateErrorOnNoInputFiles(fileNameResult: ExpandResult) {
             updateErrorForNoInputFiles(fileNameResult, this.getConfigFilePath(), this.configFileSpecs!, this.projectErrors!, this.canConfigFileJsonReportNoInputFiles);
diff --git a/src/services/types.ts b/src/services/types.ts
@@ -250,6 +250,7 @@ namespace ts {
         readFile?(path: string, encoding?: string): string | undefined;
         realpath?(path: string): string;
         fileExists?(path: string): boolean;
+        directoryExists?(directoryName: string): boolean;
 
         /*
          * LS host can optionally implement these methods to support automatic updating when new type libraries are installed
diff --git a/src/testRunner/unittests/tscWatch/programUpdates.ts b/src/testRunner/unittests/tscWatch/programUpdates.ts
@@ -812,29 +812,37 @@ declare const eval: any`
             ]
         });
 
-        verifyTscWatch({
-            scenario,
-            subScenario: "types should load from config file path if config exists",
-            commandLineArgs: ["-w", "-p", configFilePath],
-            sys: () => {
-                const f1 = {
-                    path: "/a/b/app.ts",
-                    content: "let x = 1"
-                };
-                const config = {
-                    path: configFilePath,
-                    content: JSON.stringify({ compilerOptions: { types: ["node"], typeRoots: [] } })
-                };
-                const node = {
-                    path: "/a/b/node_modules/@types/node/index.d.ts",
-                    content: "declare var process: any"
-                };
-                const cwd = {
-                    path: "/a/c"
-                };
-                return createWatchedSystem([f1, config, node, cwd, libFile], { currentDirectory: cwd.path });
-            },
-            changes: emptyArray
+        describe("types from config file", () => {
+            function verifyTypesLoad(includeTypeRoots: boolean) {
+                verifyTscWatch({
+                    scenario,
+                    subScenario: includeTypeRoots ?
+                        "types should not load from config file path if config exists but does not specifies typeRoots" :
+                        "types should load from config file path if config exists",
+                    commandLineArgs: ["-w", "-p", configFilePath],
+                    sys: () => {
+                        const f1 = {
+                            path: "/a/b/app.ts",
+                            content: "let x = 1"
+                        };
+                        const config = {
+                            path: configFilePath,
+                            content: JSON.stringify({ compilerOptions: { types: ["node"], typeRoots: includeTypeRoots ? [] : undefined } })
+                        };
+                        const node = {
+                            path: "/a/b/node_modules/@types/node/index.d.ts",
+                            content: "declare var process: any"
+                        };
+                        const cwd = {
+                            path: "/a/c"
+                        };
+                        return createWatchedSystem([f1, config, node, cwd, libFile], { currentDirectory: cwd.path });
+                    },
+                    changes: emptyArray
+                });
+            }
+            verifyTypesLoad(/*includeTypeRoots*/ false);
+            verifyTypesLoad(/*includeTypeRoots*/ true);
         });
 
         verifyTscWatch({
diff --git a/src/testRunner/unittests/tsserver/cachingFileSystemInformation.ts b/src/testRunner/unittests/tsserver/cachingFileSystemInformation.ts
@@ -391,7 +391,7 @@ namespace ts.projectSystem {
                 const { configFileName } = projectService.openClientFile(file1.path);
                 assert.equal(configFileName, tsconfigFile.path as server.NormalizedPath, `should find config`);
                 checkNumberOfConfiguredProjects(projectService, 1);
-                const watchingRecursiveDirectories = [`${canonicalFrontendDir}/src`, `${canonicalFrontendDir}/types`, `${canonicalFrontendDir}/node_modules`].concat(getNodeModuleDirectories(getDirectoryPath(canonicalFrontendDir)));
+                const watchingRecursiveDirectories = [`${canonicalFrontendDir}/src`, `${canonicalFrontendDir}/types`, `${canonicalFrontendDir}/node_modules`];
 
                 const project = projectService.configuredProjects.get(canonicalConfigPath)!;
                 verifyProjectAndWatchedDirectories();
diff --git a/src/testRunner/unittests/tsserver/resolutionCache.ts b/src/testRunner/unittests/tsserver/resolutionCache.ts
@@ -418,27 +418,37 @@ namespace ts.projectSystem {
             projectService.checkNumberOfProjects({ configuredProjects: 1 });
         });
 
-        it("types should load from config file path if config exists", () => {
-            const f1 = {
-                path: "/a/b/app.ts",
-                content: "let x = 1"
-            };
-            const config = {
-                path: "/a/b/tsconfig.json",
-                content: JSON.stringify({ compilerOptions: { types: ["node"], typeRoots: [] } })
-            };
-            const node = {
-                path: "/a/b/node_modules/@types/node/index.d.ts",
-                content: "declare var process: any"
-            };
-            const cwd = {
-                path: "/a/c"
-            };
-            const host = createServerHost([f1, config, node, cwd], { currentDirectory: cwd.path });
-            const projectService = createProjectService(host);
-            projectService.openClientFile(f1.path);
-            projectService.checkNumberOfProjects({ configuredProjects: 1 });
-            checkProjectActualFiles(configuredProjectAt(projectService, 0), [f1.path, node.path, config.path]);
+        describe("types from config file", () => {
+            function verifyTypesLoad(includeTypeRoots: boolean) {
+                const f1 = {
+                    path: "/a/b/app.ts",
+                    content: "let x = 1"
+                };
+                const config = {
+                    path: "/a/b/tsconfig.json",
+                    content: JSON.stringify({ compilerOptions: { types: ["node"], typeRoots: includeTypeRoots ? [] : undefined } })
+                };
+                const node = {
+                    path: "/a/b/node_modules/@types/node/index.d.ts",
+                    content: "declare var process: any"
+                };
+                const cwd = {
+                    path: "/a/c"
+                };
+                const host = createServerHost([f1, config, node, cwd], { currentDirectory: cwd.path });
+                const projectService = createProjectService(host);
+                projectService.openClientFile(f1.path);
+                projectService.checkNumberOfProjects({ configuredProjects: 1 });
+                checkProjectActualFiles(configuredProjectAt(projectService, 0), [f1.path, config.path, ...(includeTypeRoots ? [] : [node.path])]);
+            }
+
+            it("types should load from config file path if config exists", () => {
+                verifyTypesLoad(/*includeTypeRoots*/ false);
+            });
+
+            it("types should not load from config file path if config exists but does not specifies typeRoots", () => {
+                verifyTypesLoad(/*includeTypeRoots*/ true);
+            });
         });
     });
 
diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts
@@ -3722,7 +3722,6 @@ declare namespace ts {
         noEmitHelpers?: boolean;
     }
     export interface GetEffectiveTypeRootsHost {
-        directoryExists?(directoryName: string): boolean;
         getCurrentDirectory?(): string;
     }
     export interface TextSpan {
@@ -5334,6 +5333,7 @@ declare namespace ts {
         readFile?(path: string, encoding?: string): string | undefined;
         realpath?(path: string): string;
         fileExists?(path: string): boolean;
+        directoryExists?(directoryName: string): boolean;
         getTypeRootsVersion?(): number;
         resolveModuleNames?(moduleNames: string[], containingFile: string, reusedNames: string[] | undefined, redirectedReference: ResolvedProjectReference | undefined, options: CompilerOptions): (ResolvedModule | undefined)[];
         getResolvedModuleWithFailedLookupLocationsFromCache?(modulename: string, containingFile: string): ResolvedModuleWithFailedLookupLocations | undefined;
@@ -9304,7 +9304,6 @@ declare namespace ts.server {
         setTypeAcquisition(newTypeAcquisition: TypeAcquisition): void;
         getTypeAcquisition(): TypeAcquisition;
         close(): void;
-        getEffectiveTypeRoots(): string[];
     }
     /**
      * Project whose configuration is handled externally, such as in a '.csproj'.
diff --git a/tests/baselines/reference/api/typescript.d.ts b/tests/baselines/reference/api/typescript.d.ts
@@ -3722,7 +3722,6 @@ declare namespace ts {
         noEmitHelpers?: boolean;
     }
     export interface GetEffectiveTypeRootsHost {
-        directoryExists?(directoryName: string): boolean;
         getCurrentDirectory?(): string;
     }
     export interface TextSpan {
@@ -5334,6 +5333,7 @@ declare namespace ts {
         readFile?(path: string, encoding?: string): string | undefined;
         realpath?(path: string): string;
         fileExists?(path: string): boolean;
+        directoryExists?(directoryName: string): boolean;
         getTypeRootsVersion?(): number;
         resolveModuleNames?(moduleNames: string[], containingFile: string, reusedNames: string[] | undefined, redirectedReference: ResolvedProjectReference | undefined, options: CompilerOptions): (ResolvedModule | undefined)[];
         getResolvedModuleWithFailedLookupLocationsFromCache?(modulename: string, containingFile: string): ResolvedModuleWithFailedLookupLocations | undefined;
diff --git a/tests/baselines/reference/tscWatch/programUpdates/types-should-load-from-config-file-path-if-config-exists.js b/tests/baselines/reference/tscWatch/programUpdates/types-should-load-from-config-file-path-if-config-exists.js
@@ -3,7 +3,7 @@ Input::
 let x = 1
 
 //// [/a/b/tsconfig.json]
-{"compilerOptions":{"types":["node"],"typeRoots":[]}}
+{"compilerOptions":{"types":["node"]}}
 
 //// [/a/b/node_modules/@types/node/index.d.ts]
 declare var process: any
@@ -33,7 +33,7 @@ Output::
 
 
 Program root files: ["/a/b/app.ts"]
-Program options: {"types":["node"],"typeRoots":[],"watch":true,"project":"/a/b/tsconfig.json","configFilePath":"/a/b/tsconfig.json"}
+Program options: {"types":["node"],"watch":true,"project":"/a/b/tsconfig.json","configFilePath":"/a/b/tsconfig.json"}
 Program files::
 /a/lib/lib.d.ts
 /a/b/app.ts
diff --git a/tests/baselines/reference/tscWatch/programUpdates/types-should-not-load-from-config-file-path-if-config-exists-but-does-not-specifies-typeRoots.js b/tests/baselines/reference/tscWatch/programUpdates/types-should-not-load-from-config-file-path-if-config-exists-but-does-not-specifies-typeRoots.js
diff --git a/tests/baselines/reference/typeReferenceDirectiveWithFailedFromTypeRoot.errors.txt b/tests/baselines/reference/typeReferenceDirectiveWithFailedFromTypeRoot.errors.txt
diff --git a/tests/baselines/reference/typeReferenceDirectiveWithFailedFromTypeRoot.symbols b/tests/baselines/reference/typeReferenceDirectiveWithFailedFromTypeRoot.symbols
diff --git a/tests/baselines/reference/typeReferenceDirectiveWithFailedFromTypeRoot.trace.json b/tests/baselines/reference/typeReferenceDirectiveWithFailedFromTypeRoot.trace.json
diff --git a/tests/baselines/reference/typeReferenceDirectiveWithFailedFromTypeRoot.types b/tests/baselines/reference/typeReferenceDirectiveWithFailedFromTypeRoot.types

Original file line number	Diff line number	Diff line change
`@@ -7748,7 +7748,6 @@ namespace ts {`
`7748`	`7748`	`}`
`7749`	`7749`
`7750`	`7750`	`export interface GetEffectiveTypeRootsHost {`
`7751`		`- directoryExists?(directoryName: string): boolean;`
`7752`	`7751`	`getCurrentDirectory?(): string;`
`7753`	`7752`	`}`
`7754`	`7753`