moduleSpecifiers: Simpler criteria for preferring relative path vs baseUrl (microsoft#25803)

Andy · web-flow · commit 7b4f864b4957 · 2018-08-29T15:06:26.000-07:00
* moduleSpecifiers: Simpler criteria for preferring relative path vs baseUrl

* Don't unconditonally use a path mapping
diff --git a/src/compiler/moduleSpecifiers.ts b/src/compiler/moduleSpecifiers.ts
@@ -127,53 +127,30 @@ namespace ts.moduleSpecifiers {
         }
 
         const importRelativeToBaseUrl = removeExtensionAndIndexPostFix(relativeToBaseUrl, ending, compilerOptions);
-        if (paths) {
-            const fromPaths = tryGetModuleNameFromPaths(removeFileExtension(relativeToBaseUrl), importRelativeToBaseUrl, paths);
-            if (fromPaths) {
-                return [fromPaths];
-            }
-        }
+        const fromPaths = paths && tryGetModuleNameFromPaths(removeFileExtension(relativeToBaseUrl), importRelativeToBaseUrl, paths);
+        const nonRelative = fromPaths === undefined ? importRelativeToBaseUrl : fromPaths;
 
         if (relativePreference === RelativePreference.NonRelative) {
-            return [importRelativeToBaseUrl];
+            return [nonRelative];
         }
 
         if (relativePreference !== RelativePreference.Auto) Debug.assertNever(relativePreference);
 
-        if (isPathRelativeToParent(relativeToBaseUrl)) {
+        if (isPathRelativeToParent(nonRelative)) {
             return [relativePath];
         }
 
-        /*
-        Prefer a relative import over a baseUrl import if it doesn't traverse up to baseUrl.
-
-        Suppose we have:
-            baseUrl = /base
-            sourceDirectory = /base/a/b
-            moduleFileName = /base/foo/bar
-        Then:
-            relativePath = ../../foo/bar
-            getRelativePathNParents(relativePath) = 2
-            pathFromSourceToBaseUrl = ../../
-            getRelativePathNParents(pathFromSourceToBaseUrl) = 2
-            2 < 2 = false
-        In this case we should prefer using the baseUrl path "/a/b" instead of the relative path "../../foo/bar".
-
-        Suppose we have:
-            baseUrl = /base
-            sourceDirectory = /base/foo/a
-            moduleFileName = /base/foo/bar
-        Then:
-            relativePath = ../a
-            getRelativePathNParents(relativePath) = 1
-            pathFromSourceToBaseUrl = ../../
-            getRelativePathNParents(pathFromSourceToBaseUrl) = 2
-            1 < 2 = true
-        In this case we should prefer using the relative path "../a" instead of the baseUrl path "foo/a".
-        */
-        const pathFromSourceToBaseUrl = ensurePathIsNonModuleName(getRelativePathFromDirectory(sourceDirectory, baseUrl, getCanonicalFileName));
-        const relativeFirst = getRelativePathNParents(relativePath) < getRelativePathNParents(pathFromSourceToBaseUrl);
-        return relativeFirst ? [relativePath, importRelativeToBaseUrl] : [importRelativeToBaseUrl, 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];
+    }
+
+    function countPathComponents(path: string): number {
+        let count = 0;
+        for (let i = startsWith(path, "./") ? 2 : 0; i < path.length; i++) {
+            if (path.charCodeAt(i) === CharacterCodes.slash) count++;
+        }
+        return count;
     }
 
     function usesJsExtensionOnImports({ imports }: SourceFile): boolean {
@@ -245,15 +222,6 @@ namespace ts.moduleSpecifiers {
         return result;
     }
 
-    function getRelativePathNParents(relativePath: string): number {
-        const components = getPathComponents(relativePath);
-        if (components[0] || components.length === 1) return 0;
-        for (let i = 1; i < components.length; i++) {
-            if (components[i] !== "..") return i - 1;
-        }
-        return components.length - 1;
-    }
-
     function tryGetModuleNameFromAmbientModule(moduleSymbol: Symbol): string | undefined {
         const decl = find(moduleSymbol.declarations,
             d => isNonGlobalAmbientModule(d) && (!isExternalModuleAugmentation(d) || !isExternalModuleNameRelative(getTextOfIdentifierOrLiteral(d.name)))
diff --git a/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts b/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts
@@ -18,5 +18,8 @@
 verify.importFixAtPosition([
 `import { foo } from "a";
 
-foo();`
+foo();`,
+`import { foo } from "./folder_a/f2";
+
+foo();`,
 ]);
diff --git a/tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts b/tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts
@@ -18,5 +18,8 @@
 verify.importFixAtPosition([
 `import { foo } from "b/f2";
 
-foo();`
+foo();`,
+`import { foo } from "./folder_b/f2";
+
+foo();`,
 ]);
diff --git a/tests/cases/fourslash/importNameCodeFixNewImportPaths2.ts b/tests/cases/fourslash/importNameCodeFixNewImportPaths2.ts
@@ -24,5 +24,8 @@
 verify.importFixAtPosition([
 `import { foo } from "b";
 
-foo();`
+foo();`,
+`import { foo } from "./folder_b";
+
+foo();`,
 ]);
diff --git a/tests/cases/fourslash/importNameCodeFixNewImportPaths_withExtension.ts b/tests/cases/fourslash/importNameCodeFixNewImportPaths_withExtension.ts
@@ -19,5 +19,8 @@
 verify.importFixAtPosition([
 `import { foo } from "foo";
 
-foo`
+foo`,
+`import { foo } from "./thisHasPathMapping";
+
+foo`,
 ]);
diff --git a/tests/cases/fourslash/importNameCodeFixNewImportPaths_withLeadingDotSlash.ts b/tests/cases/fourslash/importNameCodeFixNewImportPaths_withLeadingDotSlash.ts
@@ -19,5 +19,8 @@
 verify.importFixAtPosition([
 `import { foo } from "foo";
 
-foo`
+foo`,
+`import { foo } from "./thisHasPathMapping";
+
+foo`,
 ]);
diff --git a/tests/cases/fourslash/importNameCodeFixNewImportPaths_withParentRelativePath.ts b/tests/cases/fourslash/importNameCodeFixNewImportPaths_withParentRelativePath.ts
@@ -19,5 +19,8 @@
 verify.importFixAtPosition([
 `import { foo } from "foo";
 
-foo`
+foo`,
+`import { foo } from "../thisHasPathMapping";
+
+foo`,
 ]);
diff --git a/tests/cases/fourslash/importNameCodeFix_fromPathMapping.ts b/tests/cases/fourslash/importNameCodeFix_fromPathMapping.ts
@@ -18,6 +18,9 @@
 
 goTo.file("/b.ts");
 verify.importFixAtPosition([
+`import { foo } from "./a";
+
+foo;`,
 `import { foo } from "@root/a";
 
 foo;`,
diff --git a/tests/cases/fourslash/importNameCodeFix_preferBaseUrl.ts b/tests/cases/fourslash/importNameCodeFix_preferBaseUrl.ts
@@ -0,0 +1,20 @@
+/// <reference path="fourslash.ts" />
+
+// @Filename: /tsconfig.json
+////{ "compilerOptions": { "baseUrl": "./src" } }
+
+// @Filename: /src/d0/d1/d2/file.ts
+////foo/**/;
+
+// @Filename: /src/d0/a.ts
+////export const foo = 0;
+
+goTo.file("/src/d0/d1/d2/file.ts");
+verify.importFixAtPosition([
+`import { foo } from "d0/a";
+
+foo;`,
+`import { foo } from "../../a";
+
+foo;`,
+]);
