refactor extension preference to get extension by file type

Kingwl · Kingwl · commit 607daf9d9af1 · 2018-06-23T04:11:32.000+08:00
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -4066,7 +4066,7 @@ namespace ts {
                         return `"${file.moduleName || moduleSpecifiers.getModuleSpecifiers(
                             symbol,
                             compilerOptions,
-                            contextFile,
+                            contextFile.path,
                             context!.tracker.moduleResolverHost!,
                             context!.tracker.moduleResolverHost!.getSourceFiles!(),
                             { importModuleSpecifierPreference: "non-relative" }
diff --git a/src/compiler/moduleSpecifiers.ts b/src/compiler/moduleSpecifiers.ts
@@ -3,74 +3,73 @@
 namespace ts.moduleSpecifiers {
     export interface ModuleSpecifierPreferences {
         importModuleSpecifierPreference?: "relative" | "non-relative";
-        includeExtensionInImports?: Extension.Js | Extension.Jsx;
+        includeExtensionInImports?: boolean;
     }
 
     // Note: fromSourceFile is just for usesJsExtensionOnImports
-    export function getModuleSpecifier(compilerOptions: CompilerOptions, fromSourceFile: SourceFile, fromSourceFileName: string, toFileName: string, host: ModuleSpecifierResolutionHost, preferences: ModuleSpecifierPreferences = {}) {
-        const info = getInfo(compilerOptions, fromSourceFile, fromSourceFileName, host, preferences);
-        return getGlobalModuleSpecifier(toFileName, info, host, compilerOptions) ||
+    export function getModuleSpecifier(compilerOptions: CompilerOptions, fromSourceFileName: string, toFileName: string, host: ModuleSpecifierResolutionHost, preferences: ModuleSpecifierPreferences = {}) {
+        const info = getInfo(compilerOptions, fromSourceFileName, host);
+        return getGlobalModuleSpecifier(toFileName, info, host, compilerOptions, preferences) ||
             first(getLocalModuleSpecifiers(toFileName, info, compilerOptions, preferences));
     }
 
     // For each symlink/original for a module, returns a list of ways to import that file.
     export function getModuleSpecifiers(
         moduleSymbol: Symbol,
         compilerOptions: CompilerOptions,
-        importingSourceFile: SourceFile,
+        importingSourceFileName: string,
         host: ModuleSpecifierResolutionHost,
         files: ReadonlyArray<SourceFile>,
         preferences: ModuleSpecifierPreferences,
     ): ReadonlyArray<ReadonlyArray<string>> {
         const ambient = tryGetModuleNameFromAmbientModule(moduleSymbol);
         if (ambient) return [[ambient]];
 
-        const info = getInfo(compilerOptions, importingSourceFile, importingSourceFile.path, host, preferences);
+        const info = getInfo(compilerOptions, importingSourceFileName, host);
         if (!files) {
             return Debug.fail("Files list must be present to resolve symlinks in specifier resolution");
         }
         const modulePaths = getAllModulePaths(files, getSourceFileOfNode(moduleSymbol.valueDeclaration), info.getCanonicalFileName, host);
 
-        const global = mapDefined(modulePaths, moduleFileName => getGlobalModuleSpecifier(moduleFileName, info, host, compilerOptions));
+        const global = mapDefined(modulePaths, moduleFileName => getGlobalModuleSpecifier(moduleFileName, info, host, compilerOptions, preferences));
         return global.length ? global.map(g => [g]) : modulePaths.map(moduleFileName =>
             getLocalModuleSpecifiers(moduleFileName, info, compilerOptions, preferences));
     }
 
     interface Info {
         readonly moduleResolutionKind: ModuleResolutionKind;
-        readonly addExtension: Extension.Js | Extension.Jsx | undefined;
         readonly getCanonicalFileName: GetCanonicalFileName;
         readonly sourceDirectory: string;
     }
-    // importingSourceFileName is separate because getEditsForFileRename may need to specify an updated path
-    function getInfo(compilerOptions: CompilerOptions, importingSourceFile: SourceFile, importingSourceFileName: string, host: ModuleSpecifierResolutionHost, preferences: ModuleSpecifierPreferences): Info {
+
+    function getInfo(compilerOptions: CompilerOptions, importingSourceFileName: string, host: ModuleSpecifierResolutionHost): Info {
         const moduleResolutionKind = getEmitModuleResolutionKind(compilerOptions);
-        const addExtension = usesExtensionOnImports(importingSourceFile, preferences);
         const getCanonicalFileName = createGetCanonicalFileName(host.useCaseSensitiveFileNames ? host.useCaseSensitiveFileNames() : true);
         const sourceDirectory = getDirectoryPath(importingSourceFileName);
-        return { moduleResolutionKind, addExtension, getCanonicalFileName, sourceDirectory };
+        return { moduleResolutionKind, getCanonicalFileName, sourceDirectory };
     }
 
     function getGlobalModuleSpecifier(
         moduleFileName: string,
-        { addExtension, getCanonicalFileName, sourceDirectory }: Info,
+        { getCanonicalFileName, sourceDirectory }: Info,
         host: ModuleSpecifierResolutionHost,
         compilerOptions: CompilerOptions,
+        preferences: ModuleSpecifierPreferences,
     ) {
-        return tryGetModuleNameFromTypeRoots(compilerOptions, host, getCanonicalFileName, moduleFileName, addExtension)
+        return tryGetModuleNameFromTypeRoots(compilerOptions, host, getCanonicalFileName, moduleFileName, preferences)
             || tryGetModuleNameAsNodeModule(compilerOptions, moduleFileName, host, getCanonicalFileName, sourceDirectory)
             || compilerOptions.rootDirs && tryGetModuleNameFromRootDirs(compilerOptions.rootDirs, moduleFileName, sourceDirectory, getCanonicalFileName);
     }
 
     function getLocalModuleSpecifiers(
         moduleFileName: string,
-        { moduleResolutionKind, addExtension, getCanonicalFileName, sourceDirectory }: Info,
+        { moduleResolutionKind, getCanonicalFileName, sourceDirectory }: Info,
         compilerOptions: CompilerOptions,
         preferences: ModuleSpecifierPreferences,
     ) {
         const { baseUrl, paths } = compilerOptions;
 
-        const relativePath = removeExtensionAndIndexPostFix(ensurePathIsNonModuleName(getRelativePathFromDirectory(sourceDirectory, moduleFileName, getCanonicalFileName)), moduleResolutionKind, addExtension);
+        const relativePath = removeExtensionAndIndexPostFix(ensurePathIsNonModuleName(getRelativePathFromDirectory(sourceDirectory, moduleFileName, getCanonicalFileName)), moduleResolutionKind, preferences);
         if (!baseUrl || preferences.importModuleSpecifierPreference === "relative") {
             return [relativePath];
         }
@@ -80,7 +79,7 @@ namespace ts.moduleSpecifiers {
             return [relativePath];
         }
 
-        const importRelativeToBaseUrl = removeExtensionAndIndexPostFix(relativeToBaseUrl, moduleResolutionKind, addExtension);
+        const importRelativeToBaseUrl = removeExtensionAndIndexPostFix(relativeToBaseUrl, moduleResolutionKind, preferences);
         if (paths) {
             const fromPaths = tryGetModuleNameFromPaths(removeFileExtension(relativeToBaseUrl), importRelativeToBaseUrl, paths);
             if (fromPaths) {
@@ -130,16 +129,6 @@ namespace ts.moduleSpecifiers {
         return relativeFirst ? [relativePath, importRelativeToBaseUrl] : [importRelativeToBaseUrl, relativePath];
     }
 
-    function tryGetFileExtension (text: string) {
-        return pathIsRelative(text) && fileExtensionIsOneOf(text, [Extension.Js, Extension.Jsx]) ? (
-            fileExtensionIs(text, Extension.Js) ? Extension.Js : Extension.Jsx
-        ) : undefined;
-    }
-
-    function usesExtensionOnImports({ imports }: SourceFile, preferences: ModuleSpecifierPreferences): Extension.Js | Extension.Jsx | undefined {
-        return preferences.includeExtensionInImports === undefined ? firstDefined(imports, ({ text }) => tryGetFileExtension(text)) : preferences.includeExtensionInImports;
-    }
-
     function discoverProbableSymlinks(files: ReadonlyArray<SourceFile>, getCanonicalFileName: (file: string) => string, host: ModuleSpecifierResolutionHost) {
         const symlinks = mapDefined(files, sf =>
             sf.resolvedModules && firstDefinedIterator(sf.resolvedModules.values(), res =>
@@ -251,14 +240,14 @@ namespace ts.moduleSpecifiers {
         host: GetEffectiveTypeRootsHost,
         getCanonicalFileName: (file: string) => string,
         moduleFileName: string,
-        addExtension: Extension.Js | Extension.Jsx | undefined,
+        preferences: ModuleSpecifierPreferences
     ): string | undefined {
         const roots = getEffectiveTypeRoots(options, host);
         return firstDefined(roots, unNormalizedTypeRoot => {
             const typeRoot = toPath(unNormalizedTypeRoot, /*basePath*/ undefined, getCanonicalFileName);
             if (startsWith(moduleFileName, typeRoot)) {
                 // For a type definition, we can strip `/index` even with classic resolution.
-                return removeExtensionAndIndexPostFix(moduleFileName.substring(typeRoot.length + 1), ModuleResolutionKind.NodeJs, addExtension);
+                return removeExtensionAndIndexPostFix(moduleFileName.substring(typeRoot.length + 1), ModuleResolutionKind.NodeJs, preferences);
             }
         });
     }
@@ -403,10 +392,21 @@ namespace ts.moduleSpecifiers {
         });
     }
 
-    function removeExtensionAndIndexPostFix(fileName: string, moduleResolutionKind: ModuleResolutionKind, addExtension: Extension.Js | Extension.Jsx | undefined): string {
+    function tryGetActualExtension(text: string) {
+        return pathIsRelative(text) && (
+            fileExtensionIsOneOf(text, [Extension.Js, Extension.Ts])
+                ? Extension.Js
+                : fileExtensionIsOneOf(text, [Extension.Jsx, Extension.Tsx])
+                    ? Extension.Jsx
+                    : undefined
+        );
+    }
+
+    function removeExtensionAndIndexPostFix(fileName: string, moduleResolutionKind: ModuleResolutionKind, preferences: ModuleSpecifierPreferences): string {
         const noExtension = removeFileExtension(fileName);
-        return addExtension
-            ? noExtension + addExtension
+        const actualExtension = tryGetActualExtension(fileName);
+        return (preferences.includeExtensionInImports && actualExtension)
+            ? noExtension + actualExtension
             : moduleResolutionKind === ModuleResolutionKind.NodeJs
                 ? removeSuffix(noExtension, "/index")
                 : noExtension;
diff --git a/src/server/protocol.ts b/src/server/protocol.ts
@@ -2733,7 +2733,7 @@ namespace ts.server.protocol {
         readonly includeCompletionsWithInsertText?: boolean;
         readonly importModuleSpecifierPreference?: "relative" | "non-relative";
         readonly allowTextChangesInNewFiles?: boolean;
-        readonly includeExtensionInImports?: Extension.Js | Extension.Jsx;
+        readonly includeExtensionInImports?: boolean;
     }
 
     export interface CompilerOptions {
diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts
@@ -239,7 +239,7 @@ namespace ts.codefix {
         preferences: UserPreferences,
     ): ReadonlyArray<NewImportInfo> {
         const choicesForEachExportingModule = flatMap<SymbolExportInfo, NewImportInfo[]>(moduleSymbols, ({ moduleSymbol, importKind }) => {
-            const modulePathsGroups = moduleSpecifiers.getModuleSpecifiers(moduleSymbol, program.getCompilerOptions(), sourceFile, host, program.getSourceFiles(), preferences);
+            const modulePathsGroups = moduleSpecifiers.getModuleSpecifiers(moduleSymbol, program.getCompilerOptions(), sourceFile.path, host, program.getSourceFiles(), preferences);
             return modulePathsGroups.map(group => group.map(moduleSpecifier => ({ moduleSpecifier, importKind })));
         });
         // Sort to keep the shortest paths first, but keep [relativePath, importRelativeToBaseUrl] groups together
diff --git a/src/services/getEditsForFileRename.ts b/src/services/getEditsForFileRename.ts
@@ -128,7 +128,7 @@ namespace ts {
 
                     // Need an update if the imported file moved, or the importing file moved and was using a relative path.
                     return toImport !== undefined && (toImport.updated || (importingSourceFileMoved && pathIsRelative(importLiteral.text)))
-                        ? moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), sourceFile, newImportFromPath, toImport.newFileName, host, preferences)
+                        ? moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), newImportFromPath, toImport.newFileName, host, preferences)
                         : undefined;
                 });
         }
diff --git a/src/services/types.ts b/src/services/types.ts
@@ -240,7 +240,7 @@ namespace ts {
         readonly includeCompletionsWithInsertText?: boolean;
         readonly importModuleSpecifierPreference?: "relative" | "non-relative";
         readonly allowTextChangesInNewFiles?: boolean;
-        readonly includeExtensionInImports?: Extension.Js | Extension.Jsx;
+        readonly includeExtensionInImports?: boolean;
     }
     /* @internal */
     export const defaultPreferences: UserPreferences = {};
diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts
@@ -531,7 +531,7 @@ declare namespace FourSlashInterface {
         includeCompletionsForModuleExports?: boolean;
         includeInsertTextCompletions?: boolean;
         importModuleSpecifierPreference?: "relative" | "non-relative";
-        includeExtensionInImports?: '.js' | '.jsx'
+        includeExtensionInImports?: boolean
     }
     interface CompletionsAtOptions extends UserPreferences {
         triggerCharacter?: string;
diff --git a/tests/cases/fourslash/getEditsForFileRename_jsExtension.ts b/tests/cases/fourslash/getEditsForFileRename_jsExtension.ts
@@ -13,6 +13,6 @@ verify.getEditsForFileRename({
     newPath: "/src/b.js",
     newFileContents: {
         "/b.js":
-`import { a } from "./a.js";`,
+`import { a } from "./a";`,
     },
 });
diff --git a/tests/cases/fourslash/importNameCodeFix_ExtensionPreference.ts b/tests/cases/fourslash/importNameCodeFix_ExtensionPreference.ts
@@ -0,0 +1,113 @@
+/// <reference path="fourslash.ts" />
+
+// @moduleResolution: node
+// @noLib: true
+// @allowJs: true
+// @checkJs: true
+
+// @Filename: /a.js
+////export function a() {}
+
+// @Filename: /b.ts
+////export function b() {}
+
+// @Filename: /c.jsx
+////export function c() {}
+
+// @Filename: /d.tsx
+////export function d() {}
+
+// @Filename: /normalExt.ts
+////a;
+////b;
+////c;
+////d;
+
+// @Filename: /includeExt.ts
+////a;
+////b;
+////c;
+////d;
+
+// @Filename: /includeExt.js
+////a;
+////b;
+////c;
+////d;
+
+
+goTo.file("/normalExt.ts");
+verify.importFixAtPosition([
+    `import { a } from "./a";
+
+a;
+b;
+c;
+d;`, `import { b } from "./b";
+
+a;
+b;
+c;
+d;`, `import { c } from "./c";
+
+a;
+b;
+c;
+d;`, `import { d } from "./d";
+
+a;
+b;
+c;
+d;`]);
+
+goTo.file("/includeExt.ts");
+verify.importFixAtPosition([
+    `import { a } from "./a.js";
+
+a;
+b;
+c;
+d;`, `import { b } from "./b.js";
+
+a;
+b;
+c;
+d;`, `import { c } from "./c.jsx";
+
+a;
+b;
+c;
+d;`, `import { d } from "./d.jsx";
+
+a;
+b;
+c;
+d;`], /* errorCode */ undefined, {
+        includeExtensionInImports: true
+    });
+
+goTo.file("/includeExt.js");
+verify.importFixAtPosition([
+    `import { a } from "./a.js";
+
+a;
+b;
+c;
+d;`, `import { b } from "./b.js";
+
+a;
+b;
+c;
+d;`, `import { c } from "./c.jsx";
+
+a;
+b;
+c;
+d;`, `import { d } from "./d.jsx";
+
+a;
+b;
+c;
+d;`], /* errorCode */ undefined, {
+        includeExtensionInImports: true
+    });
diff --git a/tests/cases/fourslash/importNameCodeFix_jsExtension.ts b/tests/cases/fourslash/importNameCodeFix_jsExtension.ts
@@ -20,6 +20,6 @@ verify.importFixAtPosition([
 `import * as g from "global"; // Global imports skipped
 import { a } from "./a.js";
 import { a as a2 } from "./a"; // Ignored, only the first relative import is considered
-import { b } from "./b.js";
+import { b } from "./b";
 b;`,
 ]);
diff --git a/tests/cases/fourslash/importNameCodeFix_jsExtensionPreference.ts b/tests/cases/fourslash/importNameCodeFix_jsExtensionPreference.ts

Original file line number	Diff line number	Diff line change
`@@ -2733,7 +2733,7 @@ namespace ts.server.protocol {`
`2733`	`2733`	`readonly includeCompletionsWithInsertText?: boolean;`
`2734`	`2734`	`readonly importModuleSpecifierPreference?: "relative" \| "non-relative";`
`2735`	`2735`	`readonly allowTextChangesInNewFiles?: boolean;`
`2736`		`- readonly includeExtensionInImports?: Extension.Js \| Extension.Jsx;`
	`2736`	`+ readonly includeExtensionInImports?: boolean;`
`2737`	`2737`	`}`
`2738`	`2738`
`2739`	`2739`	`export interface CompilerOptions {`
Original file line number	Diff line number	Diff line change
`@@ -128,7 +128,7 @@ namespace ts {`
`128`	`128`
`129`	`129`	`// Need an update if the imported file moved, or the importing file moved and was using a relative path.`
`130`	`130`	`return toImport !== undefined && (toImport.updated \|\| (importingSourceFileMoved && pathIsRelative(importLiteral.text)))`
`131`		`- ? moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), sourceFile, newImportFromPath, toImport.newFileName, host, preferences)`
	`131`	`+ ? moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), newImportFromPath, toImport.newFileName, host, preferences)`
`132`	`132`	`: undefined;`
`133`	`133`	`});`
`134`	`134`	`}`
Original file line number	Diff line number	Diff line change
`@@ -240,7 +240,7 @@ namespace ts {`
`240`	`240`	`readonly includeCompletionsWithInsertText?: boolean;`
`241`	`241`	`readonly importModuleSpecifierPreference?: "relative" \| "non-relative";`
`242`	`242`	`readonly allowTextChangesInNewFiles?: boolean;`
`243`		`- readonly includeExtensionInImports?: Extension.Js \| Extension.Jsx;`
	`243`	`+ readonly includeExtensionInImports?: boolean;`
`244`	`244`	`}`
`245`	`245`	`/* @internal */`
`246`	`246`	`export const defaultPreferences: UserPreferences = {};`
Original file line number	Diff line number	Diff line change
`@@ -531,7 +531,7 @@ declare namespace FourSlashInterface {`
`531`	`531`	`includeCompletionsForModuleExports?: boolean;`
`532`	`532`	`includeInsertTextCompletions?: boolean;`
`533`	`533`	`importModuleSpecifierPreference?: "relative" \| "non-relative";`
`534`		`- includeExtensionInImports?: '.js' \| '.jsx'`
	`534`	`+ includeExtensionInImports?: boolean`
`535`	`535`	`}`
`536`	`536`	`interface CompletionsAtOptions extends UserPreferences {`
`537`	`537`	`triggerCharacter?: string;`