importFixes: Only provide a fix using the best module specifier for a given module (#26738)

2026-02-14 19:16:17 -06:00 · 2018-08-29 16:18:56 -07:00 · 2018-08-29 16:18:56 -07:00 · f78dc2ad11
commit f78dc2ad11
parent a28791565d
16 changed files with 29 additions and 72 deletions
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@ -3933,7 +3933,7 @@ namespace ts {
                    // specifier preference
                    const { moduleResolverHost } = context.tracker;
                    const specifierCompilerOptions = isBundle ? { ...compilerOptions, baseUrl: moduleResolverHost.getCommonSourceDirectory() } : compilerOptions;
-                    specifier = first(first(moduleSpecifiers.getModuleSpecifiers(
+                    specifier = first(moduleSpecifiers.getModuleSpecifiers(
                        symbol,
                        specifierCompilerOptions,
                        contextFile,
@ -3941,7 +3941,7 @@ namespace ts {
                        host.getSourceFiles(),
                        { importModuleSpecifierPreference: isBundle ? "non-relative" : "relative" },
                        host.redirectTargetsMap,
-                    )));
+                    ));
                    links.specifierCache = links.specifierCache || createMap();
                    links.specifierCache.set(contextFile.path, specifier);
                }
--- a/src/compiler/moduleSpecifiers.ts
+++ b/src/compiler/moduleSpecifiers.ts
@ -75,10 +75,10 @@ namespace ts.moduleSpecifiers {
        const info = getInfo(importingSourceFileName, host);
        const modulePaths = getAllModulePaths(files, importingSourceFileName, toFileName, info.getCanonicalFileName, host, redirectTargetsMap);
        return firstDefined(modulePaths, moduleFileName => tryGetModuleNameAsNodeModule(moduleFileName, info, host, compilerOptions)) ||
-            first(getLocalModuleSpecifiers(toFileName, info, compilerOptions, preferences));
+            getLocalModuleSpecifier(toFileName, info, compilerOptions, preferences);
    }

-    // For each symlink/original for a module, returns a list of ways to import that file.
+    // Returns an import for each symlink and for the realpath.
    export function getModuleSpecifiers(
        moduleSymbol: Symbol,
        compilerOptions: CompilerOptions,
@ -87,9 +87,9 @@ namespace ts.moduleSpecifiers {
        files: ReadonlyArray<SourceFile>,
        userPreferences: UserPreferences,
        redirectTargetsMap: RedirectTargetsMap,
-    ): ReadonlyArray<ReadonlyArray<string>> {
+    ): ReadonlyArray<string> {
        const ambient = tryGetModuleNameFromAmbientModule(moduleSymbol);
-        if (ambient) return [[ambient]];
+        if (ambient) return [ambient];

        const info = getInfo(importingSourceFile.path, host);
        const moduleSourceFile = getSourceFileOfNode(moduleSymbol.valueDeclaration || getNonAugmentationDeclaration(moduleSymbol));
@ -97,8 +97,7 @@ namespace ts.moduleSpecifiers {

        const preferences = getPreferences(userPreferences, compilerOptions, importingSourceFile);
        const global = mapDefined(modulePaths, moduleFileName => tryGetModuleNameAsNodeModule(moduleFileName, info, host, compilerOptions));
-        return global.length ? global.map(g => [g]) : modulePaths.map(moduleFileName =>
-            getLocalModuleSpecifiers(moduleFileName, info, compilerOptions, preferences));
+        return global.length ? global : modulePaths.map(moduleFileName => getLocalModuleSpecifier(moduleFileName, info, compilerOptions, preferences));
    }

    interface Info {
@ -112,18 +111,18 @@ namespace ts.moduleSpecifiers {
        return { getCanonicalFileName, sourceDirectory };
    }

-    function getLocalModuleSpecifiers(moduleFileName: string, { getCanonicalFileName, sourceDirectory }: Info, compilerOptions: CompilerOptions, { ending, relativePreference }: Preferences): ReadonlyArray<string> {
+    function getLocalModuleSpecifier(moduleFileName: string, { getCanonicalFileName, sourceDirectory }: Info, compilerOptions: CompilerOptions, { ending, relativePreference }: Preferences): string {
        const { baseUrl, paths, rootDirs } = compilerOptions;

        const relativePath = rootDirs && tryGetModuleNameFromRootDirs(rootDirs, moduleFileName, sourceDirectory, getCanonicalFileName) ||
            removeExtensionAndIndexPostFix(ensurePathIsNonModuleName(getRelativePathFromDirectory(sourceDirectory, moduleFileName, getCanonicalFileName)), ending, compilerOptions);
        if (!baseUrl || relativePreference === RelativePreference.Relative) {
-            return [relativePath];
+            return relativePath;
        }

        const relativeToBaseUrl = getRelativePathIfInDirectory(moduleFileName, baseUrl, getCanonicalFileName);
        if (!relativeToBaseUrl) {
-            return [relativePath];
+            return relativePath;
        }

        const importRelativeToBaseUrl = removeExtensionAndIndexPostFix(relativeToBaseUrl, ending, compilerOptions);
@ -131,18 +130,13 @@ namespace ts.moduleSpecifiers {
        const nonRelative = fromPaths === undefined ? importRelativeToBaseUrl : fromPaths;

        if (relativePreference === RelativePreference.NonRelative) {
-            return [nonRelative];
+            return nonRelative;
        }

        if (relativePreference !== RelativePreference.Auto) Debug.assertNever(relativePreference);

-        if (isPathRelativeToParent(nonRelative)) {
-            return [relativePath];
-        }
-
        // Prefer a relative import over a baseUrl import if it has fewer components.
-        const relativeFirst = countPathComponents(relativePath) < countPathComponents(nonRelative);
-        return relativeFirst ? [relativePath, nonRelative] : [nonRelative, relativePath];
+        return isPathRelativeToParent(nonRelative) || countPathComponents(relativePath) < countPathComponents(nonRelative) ? relativePath : nonRelative;
    }

    function countPathComponents(path: string): number {
--- a/src/services/codefixes/importFixes.ts
+++ b/src/services/codefixes/importFixes.ts
@ -283,14 +283,13 @@ namespace ts.codefix {
        preferences: UserPreferences,
    ): ReadonlyArray<FixAddNewImport | FixUseImportType> {
        const isJs = isSourceFileJavaScript(sourceFile);
-        const choicesForEachExportingModule = flatMap<SymbolExportInfo, ReadonlyArray<FixAddNewImport | FixUseImportType>>(moduleSymbols, ({ moduleSymbol, importKind, exportedSymbolIsTypeOnly }) => {
-            const modulePathsGroups = moduleSpecifiers.getModuleSpecifiers(moduleSymbol, program.getCompilerOptions(), sourceFile, host, program.getSourceFiles(), preferences, program.redirectTargetsMap);
-            return modulePathsGroups.map(group => group.map((moduleSpecifier): FixAddNewImport | FixUseImportType =>
+        const choicesForEachExportingModule = flatMap(moduleSymbols, ({ moduleSymbol, importKind, exportedSymbolIsTypeOnly }) =>
+            moduleSpecifiers.getModuleSpecifiers(moduleSymbol, program.getCompilerOptions(), sourceFile, host, program.getSourceFiles(), preferences, program.redirectTargetsMap)
+            .map((moduleSpecifier): FixAddNewImport | FixUseImportType =>
                // `position` should only be undefined at a missing jsx namespace, in which case we shouldn't be looking for pure types.
                exportedSymbolIsTypeOnly && isJs ? { kind: ImportFixKind.ImportType, moduleSpecifier, position: Debug.assertDefined(position) } : { kind: ImportFixKind.AddNew, moduleSpecifier, importKind }));
-        });
-        // Sort to keep the shortest paths first, but keep [relativePath, importRelativeToBaseUrl] groups together
-        return flatten<FixAddNewImport | FixUseImportType>(choicesForEachExportingModule.sort((a, b) => first(a).moduleSpecifier.length - first(b).moduleSpecifier.length));
+        // Sort to keep the shortest paths first
+        return choicesForEachExportingModule.sort((a, b) => a.moduleSpecifier.length - b.moduleSpecifier.length);
    }

    function getFixesForAddImport(
--- a/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts
+++ b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts
@ -15,8 +15,5 @@
 verify.importFixAtPosition([
 `import { f1 } from "b";

-f1();`,
-`import { f1 } from "./a/b";
-
 f1();`,
 ]);
--- a/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl1.ts
+++ b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl1.ts
@ -14,14 +14,11 @@
 ////[|f1/*0*/();|]

 goTo.file("/a/b/y.ts");
-// Order the local import first because it's simpler.
+// Use the local import because it's simpler.
 verify.importFixAtPosition([
 `import { f1 } from "./x";

 f1();`,
-`import { f1 } from "b/x";
-
-f1();`
 ]);

 verify.importFixAtPosition([
--- a/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl2.ts
+++ b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl2.ts
@ -19,9 +19,6 @@ verify.importFixAtPosition([
 `import { f1 } from "b/x";

 f1();`,
-`import { f1 } from "../b/x";
-
-f1();`
 ]);

 verify.importFixAtPosition([
--- a/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts
+++ b/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts
@ -18,8 +18,5 @@
 verify.importFixAtPosition([
 `import { foo } from "a";

-foo();`,
-`import { foo } from "./folder_a/f2";
-
-foo();`,
+foo();`
 ]);
--- a/tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts
+++ b/tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts
@ -18,8 +18,5 @@
 verify.importFixAtPosition([
 `import { foo } from "b/f2";

-foo();`,
-`import { foo } from "./folder_b/f2";
-
-foo();`,
+foo();`
 ]);
--- a/tests/cases/fourslash/importNameCodeFixNewImportPaths2.ts
+++ b/tests/cases/fourslash/importNameCodeFixNewImportPaths2.ts
@ -24,8 +24,5 @@
 verify.importFixAtPosition([
 `import { foo } from "b";

-foo();`,
-`import { foo } from "./folder_b";
-
-foo();`,
+foo();`
 ]);
--- a/tests/cases/fourslash/importNameCodeFixNewImportPaths_withExtension.ts
+++ b/tests/cases/fourslash/importNameCodeFixNewImportPaths_withExtension.ts
@ -19,8 +19,5 @@
 verify.importFixAtPosition([
 `import { foo } from "foo";

-foo`,
-`import { foo } from "./thisHasPathMapping";
-
-foo`,
+foo`
 ]);
--- a/tests/cases/fourslash/importNameCodeFixNewImportPaths_withLeadingDotSlash.ts
+++ b/tests/cases/fourslash/importNameCodeFixNewImportPaths_withLeadingDotSlash.ts
@ -19,8 +19,5 @@
 verify.importFixAtPosition([
 `import { foo } from "foo";

-foo`,
-`import { foo } from "./thisHasPathMapping";
-
-foo`,
+foo`
 ]);
--- a/tests/cases/fourslash/importNameCodeFixNewImportPaths_withParentRelativePath.ts
+++ b/tests/cases/fourslash/importNameCodeFixNewImportPaths_withParentRelativePath.ts
@ -19,8 +19,5 @@
 verify.importFixAtPosition([
 `import { foo } from "foo";

-foo`,
-`import { foo } from "../thisHasPathMapping";
-
-foo`,
+foo`
 ]);
--- a/tests/cases/fourslash/importNameCodeFixNewImportTypeRoots1.ts
+++ b/tests/cases/fourslash/importNameCodeFixNewImportTypeRoots1.ts
@ -16,12 +16,9 @@
 ////     }
 //// }

-// "typeRoots" does not affect module resolution. Importing from "random" would be a compile error.
+// "typeRoots" does not affect module resolution, though "baseUrl" does. Importing from "random" would be a compile error.
 verify.importFixAtPosition([
 `import { foo } from "types/random";

 foo();`,
-`import { foo } from "../types/random";
-
-foo();`
 ]);
--- a/tests/cases/fourslash/importNameCodeFix_fromPathMapping.ts
+++ b/tests/cases/fourslash/importNameCodeFix_fromPathMapping.ts
@ -3,7 +3,7 @@
 // @Filename: /a.ts
 ////export const foo = 0;

-// @Filename: /b.ts
+// @Filename: /x/y.ts
 ////foo;

 // @Filename: /tsconfig.json
@ -16,11 +16,8 @@
 ////    }
 ////}

-goTo.file("/b.ts");
+goTo.file("/x/y.ts");
 verify.importFixAtPosition([
-`import { foo } from "./a";
-
-foo;`,
 `import { foo } from "@root/a";

 foo;`,
--- a/tests/cases/fourslash/importNameCodeFix_preferBaseUrl.ts
+++ b/tests/cases/fourslash/importNameCodeFix_preferBaseUrl.ts
@ -13,8 +13,5 @@ goTo.file("/src/d0/d1/d2/file.ts");
 verify.importFixAtPosition([
 `import { foo } from "d0/a";

-foo;`,
-`import { foo } from "../../a";
-
-foo;`,
+foo;`
 ]);
--- a/tests/cases/fourslash/importNameCodeFix_rootDirs.ts
+++ b/tests/cases/fourslash/importNameCodeFix_rootDirs.ts
@ -18,6 +18,6 @@ const nonRelative = 'import { a } from "a";\n\na;';
 const relative = nonRelative.replace('"a"', '"./a"');

 goTo.file("/b.ts");
-verify.importFixAtPosition([nonRelative, relative]);
+verify.importFixAtPosition([nonRelative]);
 verify.importFixAtPosition([nonRelative], undefined, { importModuleSpecifierPreference: "non-relative" });
 verify.importFixAtPosition([relative], undefined, { importModuleSpecifierPreference: "relative" });