Redo resolution on ATA when previous resolution was to '.js' file (#28236)

* Redo resolution on ATA when previous resolution was to '.js' file

* Use a separate test case

Author: Andy

src/compiler/resolutionCache.ts

@@ -248,6 +248,7 @@ namespace ts {
             perDirectoryCacheWithRedirects: CacheWithRedirects<Map<T>>,
             loader: (name: string, containingFile: string, options: CompilerOptions, host: ModuleResolutionHost, redirectedReference?: ResolvedProjectReference) => T,
             getResolutionWithResolvedFileName: GetResolutionWithResolvedFileName<T, R>,
+            shouldRetryResolution: (t: T) => boolean,
             reusedNames: ReadonlyArray<string> | undefined,
             logChanges: boolean): (R | undefined)[] {
 
@@ -260,7 +261,7 @@ namespace ts {
                 perDirectoryResolution = createMap();
                 perDirectoryCache.set(dirPath, perDirectoryResolution);
             }
-            const resolvedModules: R[] = [];
+            const resolvedModules: (R | undefined)[] = [];
             const compilerOptions = resolutionHost.getCompilationSettings();
             const hasInvalidatedNonRelativeUnresolvedImport = logChanges && isFileWithInvalidatedNonRelativeUnresolvedImports(path);
 
@@ -278,7 +279,7 @@ namespace ts {
                 if (!seenNamesInFile.has(name) &&
                     allFilesHaveInvalidatedResolution || unmatchedRedirects || !resolution || resolution.isInvalidated ||
                     // If the name is unresolved import that was invalidated, recalculate
-                    (hasInvalidatedNonRelativeUnresolvedImport && !isExternalModuleNameRelative(name) && !getResolutionWithResolvedFileName(resolution))) {
+                    (hasInvalidatedNonRelativeUnresolvedImport && !isExternalModuleNameRelative(name) && shouldRetryResolution(resolution))) {
                     const existingResolution = resolution;
                     const resolutionInDirectory = perDirectoryResolution.get(name);
                     if (resolutionInDirectory) {
@@ -302,7 +303,7 @@ namespace ts {
                 }
                 Debug.assert(resolution !== undefined && !resolution.isInvalidated);
                 seenNamesInFile.set(name, true);
-                resolvedModules.push(getResolutionWithResolvedFileName(resolution)!); // TODO: GH#18217
+                resolvedModules.push(getResolutionWithResolvedFileName(resolution));
             }
 
             // Stop watching and remove the unused name
@@ -339,6 +340,7 @@ namespace ts {
                 typeDirectiveNames, containingFile, redirectedReference,
                 resolvedTypeReferenceDirectives, perDirectoryResolvedTypeReferenceDirectives,
                 resolveTypeReferenceDirective, getResolvedTypeReferenceDirective,
+                /*shouldRetryResolution*/ resolution => resolution.resolvedTypeReferenceDirective === undefined,
                 /*reusedNames*/ undefined, /*logChanges*/ false
             );
         }
@@ -348,6 +350,7 @@ namespace ts {
                 moduleNames, containingFile, redirectedReference,
                 resolvedModuleNames, perDirectoryResolvedModuleNames,
                 resolveModuleName, getResolvedModule,
+                /*shouldRetryResolution*/ resolution => !resolution.resolvedModule || !resolutionExtensionIsTSOrJson(resolution.resolvedModule.extension),
                 reusedNames, logChangesWhenResolvingModule
             );
         }

src/server/project.ts

@@ -1186,7 +1186,9 @@ namespace ts.server {
             let unresolvedImports: string[] | undefined;
             file.resolvedModules.forEach((resolvedModule, name) => {
                 // pick unresolved non-relative names
-                if (!resolvedModule && !isExternalModuleNameRelative(name) && !ambientModules.some(m => m === name)) {
+                if ((!resolvedModule || !resolutionExtensionIsTSOrJson(resolvedModule.extension)) &&
+                    !isExternalModuleNameRelative(name) &&
+                    !ambientModules.some(m => m === name)) {
                     unresolvedImports = append(unresolvedImports, parsePackageName(name).packageName);
                 }
             });

src/testRunner/unittests/typingsInstaller.ts

@@ -986,6 +986,50 @@ namespace ts.projectSystem {
             checkProjectActualFiles(service.inferredProjects[0], [file.path, node.path, commander.path]);
         });
 
+        it("should redo resolution that resolved to '.js' file after typings are installed", () => {
+            const file: TestFSWithWatch.File = {
+                path: "/a/b/app.js",
+                content: `
+                import * as commander from "commander";`
+            };
+            const cachePath = "/a/cache";
+            const commanderJS: TestFSWithWatch.File = {
+                path: "/node_modules/commander/index.js",
+                content: "module.exports = 0",
+            };
+
+            const typeNames: ReadonlyArray<string> = ["commander"];
+            const typePath = (name: string): string => `${cachePath}/node_modules/@types/${name}/index.d.ts`;
+            const host = createServerHost([file, commanderJS]);
+            const installer = new (class extends Installer {
+                constructor() {
+                    super(host, { globalTypingsCacheLocation: cachePath, typesRegistry: createTypesRegistry(...typeNames) });
+                }
+                installWorker(_requestId: number, _args: string[], _cwd: string, cb: TI.RequestCompletedAction) {
+                    const installedTypings = typeNames.map(name => `@types/${name}`);
+                    const typingFiles = typeNames.map((name): TestFSWithWatch.File => ({ path: typePath(name), content: "" }));
+                    executeCommand(this, host, installedTypings, typingFiles, cb);
+                }
+            })();
+            const service = createProjectService(host, { typingsInstaller: installer });
+            service.openClientFile(file.path);
+
+            checkWatchedFiles(host, [...flatMap(["/a/b", "/a", ""], x => [x + "/tsconfig.json", x + "/jsconfig.json"]), "/a/lib/lib.d.ts"]);
+            checkWatchedDirectories(host, [], /*recursive*/ false);
+            // Does not include cachePath because that is handled by typingsInstaller
+            checkWatchedDirectories(host, ["/node_modules", "/a/b/node_modules", "/a/b/node_modules/@types", "/a/b/bower_components"], /*recursive*/ true);
+
+            service.checkNumberOfProjects({ inferredProjects: 1 });
+            checkProjectActualFiles(service.inferredProjects[0], [file.path, commanderJS.path]);
+
+            installer.installAll(/*expectedCount*/1);
+            for (const name of typeNames) {
+                assert.isTrue(host.fileExists(typePath(name)), `typings for '${name}' should be created`);
+            }
+            host.checkTimeoutQueueLengthAndRun(2);
+            checkProjectActualFiles(service.inferredProjects[0], [file.path, ...typeNames.map(typePath)]);
+        });
+
         it("should pick typing names from non-relative unresolved imports", () => {
             const f1 = {
                 path: "/a/b/app.js",