From 2386f271b96a14e9bf60dbdcce1aab154c9a201e Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Thu, 19 Jul 2018 10:13:00 -0700 Subject: [PATCH 1/2] moduleSpecifiers: Simpler criteria for preferring relative path vs baseUrl --- src/compiler/moduleSpecifiers.ts | 56 +++++-------------- .../importNameCodeFix_preferBaseUrl.ts | 20 +++++++ 2 files changed, 33 insertions(+), 43 deletions(-) create mode 100644 tests/cases/fourslash/importNameCodeFix_preferBaseUrl.ts diff --git a/src/compiler/moduleSpecifiers.ts b/src/compiler/moduleSpecifiers.ts index 11559815412e7..b80721b5eaec1 100644 --- a/src/compiler/moduleSpecifiers.ts +++ b/src/compiler/moduleSpecifiers.ts @@ -89,11 +89,9 @@ namespace ts.moduleSpecifiers { } const importRelativeToBaseUrl = removeExtensionAndIndexPostFix(relativeToBaseUrl, moduleResolutionKind, addJsExtension); - if (paths) { - const fromPaths = tryGetModuleNameFromPaths(removeFileExtension(relativeToBaseUrl), importRelativeToBaseUrl, paths); - if (fromPaths) { - return [fromPaths]; - } + const fromPaths = paths && tryGetModuleNameFromPaths(removeFileExtension(relativeToBaseUrl), importRelativeToBaseUrl, paths); + if (fromPaths) { + return [fromPaths]; } if (preferences.importModuleSpecifierPreference === "non-relative") { @@ -106,38 +104,19 @@ namespace ts.moduleSpecifiers { 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); + // Prefer a relative import over a baseUrl import if it has fewer components. + const relativeFirst = countPathComponents(relativePath) < countPathComponents(importRelativeToBaseUrl); return relativeFirst ? [relativePath, importRelativeToBaseUrl] : [importRelativeToBaseUrl, 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 { return firstDefined(imports, ({ text }) => pathIsRelative(text) ? fileExtensionIs(text, Extension.Js) : undefined) || false; } @@ -197,15 +176,6 @@ namespace ts.moduleSpecifiers { return symlinks.length === 0 ? getAllModulePathsUsingIndirectSymlinks(files, getNormalizedAbsolutePath(importedFileName, host.getCurrentDirectory ? host.getCurrentDirectory() : ""), getCanonicalFileName, host) : symlinks; } - 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 = moduleSymbol.valueDeclaration; if (isModuleDeclaration(decl) && isStringLiteral(decl.name)) { diff --git a/tests/cases/fourslash/importNameCodeFix_preferBaseUrl.ts b/tests/cases/fourslash/importNameCodeFix_preferBaseUrl.ts new file mode 100644 index 0000000000000..5d2d861550e26 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFix_preferBaseUrl.ts @@ -0,0 +1,20 @@ +/// + +// @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;`, +]); From 1f51140359b8984617e71ff47c9cb9f178299ba2 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Thu, 9 Aug 2018 17:03:02 -0700 Subject: [PATCH 2/2] Don't unconditonally use a path mapping --- src/compiler/moduleSpecifiers.ts | 16 ++++++---------- .../importNameCodeFixNewImportPaths0.ts | 5 ++++- .../importNameCodeFixNewImportPaths1.ts | 5 ++++- .../importNameCodeFixNewImportPaths2.ts | 5 ++++- ...ortNameCodeFixNewImportPaths_withExtension.ts | 5 ++++- ...eCodeFixNewImportPaths_withLeadingDotSlash.ts | 5 ++++- ...deFixNewImportPaths_withParentRelativePath.ts | 5 ++++- .../importNameCodeFix_fromPathMapping.ts | 3 +++ 8 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/compiler/moduleSpecifiers.ts b/src/compiler/moduleSpecifiers.ts index a6f5981319e2c..553a7781851ef 100644 --- a/src/compiler/moduleSpecifiers.ts +++ b/src/compiler/moduleSpecifiers.ts @@ -87,11 +87,9 @@ namespace ts.moduleSpecifiers { function getLocalModuleSpecifiers( moduleFileName: string, { moduleResolutionKind, addJsExtension, getCanonicalFileName, sourceDirectory }: Info, - compilerOptions: CompilerOptions, + { baseUrl, paths, rootDirs }: CompilerOptions, preferences: ModuleSpecifierPreferences, ): ReadonlyArray { - const { baseUrl, paths, rootDirs } = compilerOptions; - const relativePath = rootDirs && tryGetModuleNameFromRootDirs(rootDirs, moduleFileName, sourceDirectory, getCanonicalFileName) || removeExtensionAndIndexPostFix(ensurePathIsNonModuleName(getRelativePathFromDirectory(sourceDirectory, moduleFileName, getCanonicalFileName)), moduleResolutionKind, addJsExtension); if (!baseUrl || preferences.importModuleSpecifierPreference === "relative") { @@ -105,23 +103,21 @@ namespace ts.moduleSpecifiers { const importRelativeToBaseUrl = removeExtensionAndIndexPostFix(relativeToBaseUrl, moduleResolutionKind, addJsExtension); const fromPaths = paths && tryGetModuleNameFromPaths(removeFileExtension(relativeToBaseUrl), importRelativeToBaseUrl, paths); - if (fromPaths) { - return [fromPaths]; - } + const nonRelative = fromPaths === undefined ? importRelativeToBaseUrl : fromPaths; if (preferences.importModuleSpecifierPreference === "non-relative") { - return [importRelativeToBaseUrl]; + return [nonRelative]; } if (preferences.importModuleSpecifierPreference !== undefined) Debug.assertNever(preferences.importModuleSpecifierPreference); - if (isPathRelativeToParent(relativeToBaseUrl)) { + if (isPathRelativeToParent(nonRelative)) { return [relativePath]; } // Prefer a relative import over a baseUrl import if it has fewer components. - const relativeFirst = countPathComponents(relativePath) < countPathComponents(importRelativeToBaseUrl); - return relativeFirst ? [relativePath, importRelativeToBaseUrl] : [importRelativeToBaseUrl, relativePath]; + const relativeFirst = countPathComponents(relativePath) < countPathComponents(nonRelative); + return relativeFirst ? [relativePath, nonRelative] : [nonRelative, relativePath]; } function countPathComponents(path: string): number { diff --git a/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts b/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts index bd3fd9a84b3f5..90f2170332617 100644 --- 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 index 5ac195e9ab468..041ef40380dc8 100644 --- 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 index 33b8d9330be71..63aabfc4e5f79 100644 --- 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 index c383b09862e2e..2275758cda160 100644 --- 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 index f29932eed732b..acf1fa369ef79 100644 --- 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 index 4bd938ae0b97a..816fb309a0837 100644 --- 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 index dda8b8551234a..35a42d8cbf1df 100644 --- 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;`,