Add new importModuleSpecifierPreference value

andrewbranch · andrewbranch · commit 8571c638d4c4 · 2020-09-17T12:42:33.000-07:00
diff --git a/src/compiler/moduleSpecifiers.ts b/src/compiler/moduleSpecifiers.ts
@@ -1,7 +1,7 @@
 // Used by importFixes, getEditsForFileRename, and declaration emit to synthesize import module specifiers.
 /* @internal */
 namespace ts.moduleSpecifiers {
-    const enum RelativePreference { Relative, NonRelative, Auto }
+    const enum RelativePreference { Relative, NonRelative, Shortest, ExternalNonRelative }
     // See UserPreferences#importPathEnding
     const enum Ending { Minimal, Index, JsExtension }
 
@@ -13,7 +13,11 @@ namespace ts.moduleSpecifiers {
 
     function getPreferences({ importModuleSpecifierPreference, importModuleSpecifierEnding }: UserPreferences, compilerOptions: CompilerOptions, importingSourceFile: SourceFile): Preferences {
         return {
-            relativePreference: importModuleSpecifierPreference === "relative" ? RelativePreference.Relative : importModuleSpecifierPreference === "non-relative" ? RelativePreference.NonRelative : RelativePreference.Auto,
+            relativePreference:
+                importModuleSpecifierPreference === "relative" ? RelativePreference.Relative :
+                importModuleSpecifierPreference === "non-relative" ? RelativePreference.NonRelative :
+                importModuleSpecifierPreference === "external-non-relative" ? RelativePreference.ExternalNonRelative :
+                RelativePreference.Shortest,
             ending: getEnding(),
         };
         function getEnding(): Ending {
@@ -147,17 +151,19 @@ namespace ts.moduleSpecifiers {
 
     interface Info {
         readonly getCanonicalFileName: GetCanonicalFileName;
+        readonly importingSourceFileName: Path
         readonly sourceDirectory: Path;
     }
     // importingSourceFileName is separate because getEditsForFileRename may need to specify an updated path
     function getInfo(importingSourceFileName: Path, host: ModuleSpecifierResolutionHost): Info {
         const getCanonicalFileName = createGetCanonicalFileName(host.useCaseSensitiveFileNames ? host.useCaseSensitiveFileNames() : true);
         const sourceDirectory = getDirectoryPath(importingSourceFileName);
-        return { getCanonicalFileName, sourceDirectory };
+        return { getCanonicalFileName, importingSourceFileName, sourceDirectory };
     }
 
-    function getLocalModuleSpecifier(moduleFileName: string, { getCanonicalFileName, sourceDirectory }: Info, compilerOptions: CompilerOptions, host: ModuleSpecifierResolutionHost, { ending, relativePreference }: Preferences): string {
+    function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOptions: CompilerOptions, host: ModuleSpecifierResolutionHost, { ending, relativePreference }: Preferences): string {
         const { baseUrl, paths, rootDirs, bundledPackageName } = compilerOptions;
+        const { sourceDirectory, getCanonicalFileName } = info;
 
         const relativePath = rootDirs && tryGetModuleNameFromRootDirs(rootDirs, moduleFileName, sourceDirectory, getCanonicalFileName, ending, compilerOptions) ||
             removeExtensionAndIndexPostFix(ensurePathIsNonModuleName(getRelativePathFromDirectory(sourceDirectory, moduleFileName, getCanonicalFileName)), ending, compilerOptions);
@@ -174,13 +180,34 @@ namespace ts.moduleSpecifiers {
         const bundledPkgReference = bundledPackageName ? combinePaths(bundledPackageName, relativeToBaseUrl) : relativeToBaseUrl;
         const importRelativeToBaseUrl = removeExtensionAndIndexPostFix(bundledPkgReference, ending, compilerOptions);
         const fromPaths = paths && tryGetModuleNameFromPaths(removeFileExtension(bundledPkgReference), importRelativeToBaseUrl, paths);
-        const nonRelative = fromPaths === undefined ? importRelativeToBaseUrl : fromPaths;
+        const nonRelative = fromPaths === undefined ? importRelativeToBaseUrl : fromPaths.path;
 
         if (relativePreference === RelativePreference.NonRelative) {
             return nonRelative;
         }
 
-        if (relativePreference !== RelativePreference.Auto) Debug.assertNever(relativePreference);
+        if (relativePreference === RelativePreference.ExternalNonRelative) {
+            Debug.assertIsDefined(host.getNearestAncestorDirectoryWithPackageJson);
+            const projectDirectory = host.getCurrentDirectory();
+            const modulePath = toPath(moduleFileName, projectDirectory, getCanonicalFileName);
+            const sourceIsInternal = startsWith(sourceDirectory, projectDirectory);
+            const targetIsInternal = startsWith(modulePath, projectDirectory);
+            if (sourceIsInternal && !targetIsInternal || !sourceIsInternal && targetIsInternal) {
+                // 1. The import path crosses the boundary of the tsconfig.json-containing directory.
+                return nonRelative;
+            }
+
+            const nearestTargetPackageJson = host.getNearestAncestorDirectoryWithPackageJson(getDirectoryPath(modulePath));
+            const nearestSourcePackageJson = host.getNearestAncestorDirectoryWithPackageJson(sourceDirectory);
+            if (nearestSourcePackageJson !== nearestTargetPackageJson) {
+                // 2. The importing and imported files are part of different packages.
+                return nonRelative;
+            }
+
+            return relativePath;
+        }
+
+        if (relativePreference !== RelativePreference.Shortest) Debug.assertNever(relativePreference);
 
         // Prefer a relative import over a baseUrl import if it has fewer components.
         return isPathRelativeToParent(nonRelative) || countPathComponents(relativePath) < countPathComponents(nonRelative) ? relativePath : nonRelative;
@@ -325,7 +352,7 @@ namespace ts.moduleSpecifiers {
         }
     }
 
-    function tryGetModuleNameFromPaths(relativeToBaseUrlWithIndex: string, relativeToBaseUrl: string, paths: MapLike<readonly string[]>): string | undefined {
+    function tryGetModuleNameFromPaths(relativeToBaseUrlWithIndex: string, relativeToBaseUrl: string, paths: MapLike<readonly string[]>): { key: string, path: string } | undefined {
         for (const key in paths) {
             for (const patternText of paths[key]) {
                 const pattern = removeFileExtension(normalizePath(patternText));
@@ -338,11 +365,11 @@ namespace ts.moduleSpecifiers {
                         endsWith(relativeToBaseUrl, suffix) ||
                         !suffix && relativeToBaseUrl === removeTrailingDirectorySeparator(prefix)) {
                         const matchedStar = relativeToBaseUrl.substr(prefix.length, relativeToBaseUrl.length - suffix.length);
-                        return key.replace("*", matchedStar);
+                        return { key, path: key.replace("*", matchedStar) };
                     }
                 }
                 else if (pattern === relativeToBaseUrl || pattern === relativeToBaseUrlWithIndex) {
-                    return key;
+                    return { key, path: key };
                 }
             }
         }
@@ -431,7 +458,7 @@ namespace ts.moduleSpecifiers {
                         versionPaths.paths
                     );
                     if (fromPaths !== undefined) {
-                        moduleFileToTry = combinePaths(packageRootPath, fromPaths);
+                        moduleFileToTry = combinePaths(packageRootPath, fromPaths.path);
                     }
                 }
 
diff --git a/src/compiler/types.ts b/src/compiler/types.ts
@@ -7847,6 +7847,7 @@ namespace ts {
         realpath?(path: string): string;
         getSymlinkCache?(): SymlinkCache;
         getGlobalTypingsCacheLocation?(): string | undefined;
+        getNearestAncestorDirectoryWithPackageJson?(fileName: string, rootDir?: string): string | undefined;
 
         getSourceFiles(): readonly SourceFile[];
         readonly redirectTargetsMap: RedirectTargetsMap;
@@ -8148,7 +8149,7 @@ namespace ts {
         readonly includeCompletionsForModuleExports?: boolean;
         readonly includeAutomaticOptionalChainCompletions?: boolean;
         readonly includeCompletionsWithInsertText?: boolean;
-        readonly importModuleSpecifierPreference?: "auto" | "relative" | "non-relative";
+        readonly importModuleSpecifierPreference?: "shortest" | "external-non-relative" | "relative" | "non-relative";
         /** Determines whether we import `foo/index.ts` as "foo", "foo/index", or "foo/index.js" */
         readonly importModuleSpecifierEnding?: "auto" | "minimal" | "index" | "js";
         readonly allowTextChangesInNewFiles?: boolean;
diff --git a/src/harness/fourslashImpl.ts b/src/harness/fourslashImpl.ts
@@ -2927,6 +2927,10 @@ namespace FourSlash {
             }
             const range = ts.firstOrUndefined(ranges);
 
+            if (preferences) {
+                this.configure(preferences);
+            }
+
             const codeFixes = this.getCodeFixes(fileName, errorCode, preferences).filter(f => f.fixName === ts.codefix.importFixName);
 
             if (codeFixes.length === 0) {
diff --git a/src/server/editorServices.ts b/src/server/editorServices.ts
@@ -3778,7 +3778,7 @@ namespace ts.server {
                         const info = packageJsonCache.getInDirectory(directory);
                         if (info) result.push(info);
                 }
-                if (rootPath && rootPath === this.toPath(directory)) {
+                if (rootPath && rootPath === directory) {
                     return true;
                 }
             };
@@ -3787,6 +3787,20 @@ namespace ts.server {
             return result;
         }
 
+        /*@internal*/
+        getNearestAncestorDirectoryWithPackageJson(fileName: string): string | undefined {
+            return forEachAncestorDirectory(fileName, directory => {
+                switch (this.packageJsonCache.directoryHasPackageJson(this.toPath(directory))) {
+                    case Ternary.True: return directory;
+                    case Ternary.False: return undefined;
+                    case Ternary.Maybe:
+                        return this.host.fileExists(combinePaths(directory, "package.json"))
+                            ? directory
+                            : undefined;
+                }
+            });
+        }
+
         /*@internal*/
         private watchPackageJsonFile(path: Path) {
             const watchers = this.packageJsonFilesMap || (this.packageJsonFilesMap = new Map());
diff --git a/src/server/project.ts b/src/server/project.ts
@@ -1625,6 +1625,11 @@ namespace ts.server {
             return this.projectService.getPackageJsonsVisibleToFile(fileName, rootDir);
         }
 
+        /*@internal*/
+        getNearestAncestorDirectoryWithPackageJson(fileName: string): string | undefined {
+            return this.projectService.getNearestAncestorDirectoryWithPackageJson(fileName);
+        }
+
         /*@internal*/
         getPackageJsonsForAutoImport(rootDir?: string): readonly PackageJsonInfo[] {
             const packageJsons = this.getPackageJsonsVisibleToFile(combinePaths(this.currentDirectory, inferredTypesContainingFile), rootDir);
@@ -1955,6 +1960,10 @@ namespace ts.server {
             return super.updateGraph();
         }
 
+        hasRoots() {
+            return !!this.rootFileNames?.length;
+        }
+
         markAsDirty() {
             this.rootFileNames = undefined;
             super.markAsDirty();
diff --git a/src/server/protocol.ts b/src/server/protocol.ts
@@ -3217,7 +3217,7 @@ namespace ts.server.protocol {
          * values, with insertion text to replace preceding `.` tokens with `?.`.
          */
         readonly includeAutomaticOptionalChainCompletions?: boolean;
-        readonly importModuleSpecifierPreference?: "auto" | "relative" | "non-relative";
+        readonly importModuleSpecifierPreference?: "shortest" | "external-non-relative" | "relative" | "non-relative";
         /** Determines whether we import `foo/index.ts` as "foo", "foo/index", or "foo/index.js" */
         readonly importModuleSpecifierEnding?: "auto" | "minimal" | "index" | "js";
         readonly allowTextChangesInNewFiles?: boolean;
diff --git a/src/services/types.ts b/src/services/types.ts
@@ -301,6 +301,8 @@ namespace ts {
         /* @internal */
         getPackageJsonsVisibleToFile?(fileName: string, rootDir?: string): readonly PackageJsonInfo[];
         /* @internal */
+        getNearestAncestorDirectoryWithPackageJson?(fileName: string, rootDir?: string): string | undefined;
+        /* @internal */
         getPackageJsonsForAutoImport?(rootDir?: string): readonly PackageJsonInfo[];
         /* @internal */
         getImportSuggestionsCache?(): Completions.ImportSuggestionsForFileCache;
diff --git a/src/services/utilities.ts b/src/services/utilities.ts
@@ -1773,7 +1773,8 @@ namespace ts {
             redirectTargetsMap: program.redirectTargetsMap,
             getProjectReferenceRedirect: fileName => program.getProjectReferenceRedirect(fileName),
             isSourceOfProjectReferenceRedirect: fileName => program.isSourceOfProjectReferenceRedirect(fileName),
-            getCompilerOptions: () => program.getCompilerOptions()
+            getCompilerOptions: () => program.getCompilerOptions(),
+            getNearestAncestorDirectoryWithPackageJson: maybeBind(host, host.getNearestAncestorDirectoryWithPackageJson),
         };
     }
 
diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts
@@ -610,7 +610,7 @@ declare namespace FourSlashInterface {
         readonly includeCompletionsForModuleExports?: boolean;
         readonly includeInsertTextCompletions?: boolean;
         readonly includeAutomaticOptionalChainCompletions?: boolean;
-        readonly importModuleSpecifierPreference?: "auto" | "relative" | "non-relative";
+        readonly importModuleSpecifierPreference?: "shortest" | "external-non-relative" | "relative" | "non-relative";
         readonly importModuleSpecifierEnding?: "minimal" | "index" | "js";
     }
     interface CompletionsOptions {
diff --git a/tests/cases/fourslash/server/importNameCodeFix_externalNonRelative1.ts b/tests/cases/fourslash/server/importNameCodeFix_externalNonRelative1.ts
@@ -0,0 +1,58 @@
+/// <reference path="../fourslash.ts" />
+
+// @Filename: /tsconfig.base.json
+//// {
+////   "compilerOptions": {
+////     "module": "commonjs",
+////     "paths": {
+////       "pkg-1/*": ["./packages/pkg-1/src/*"],
+////       "pkg-2/*": ["./packages/pkg-2/src/*"]
+////     }
+////   }
+//// }
+
+// @Filename: /packages/pkg-1/package.json
+//// { "dependencies": { "pkg-2": "*" } }
+
+// @Filename: /packages/pkg-1/tsconfig.json
+//// {
+////   "extends": "../../tsconfig.base.json",
+////   "references": [
+////     { "path": "../pkg-2" }
+////   ]
+//// }
+
+// @Filename: /packages/pkg-1/src/index.ts
+//// Pkg2/*external*/
+
+// @Filename: /packages/pkg-2/package.json
+//// { "types": "dist/index.d.ts" }
+
+// @Filename: /packages/pkg-2/tsconfig.json
+//// {
+////   "extends": "../../tsconfig.base.json",
+////   "compilerOptions": { "outDir": "dist", "rootDir": "src", "composite": true }
+//// }
+
+// @Filename: /packages/pkg-2/src/index.ts
+//// import "./utils";
+
+// @Filename: /packages/pkg-2/src/utils.ts
+//// export const Pkg2 = {};
+
+// @Filename: /packages/pkg-2/src/blah/foo/data.ts
+//// Pkg2/*internal*/
+
+// @link: /packages/pkg-2 -> /packages/pkg-1/node_modules/pkg-2
+
+format.setOption("newline", "\n");
+
+goTo.marker("external");
+verify.importFixAtPosition([`import { Pkg2 } from "pkg-2/utils";\n\nPkg2`], /*errorCode*/ undefined, {
+  importModuleSpecifierPreference: "external-non-relative"
+});
+
+goTo.marker("internal");
+verify.importFixAtPosition([`import { Pkg2 } from "../../utils";\n\nPkg2`], /*errorCode*/ undefined, {
+  importModuleSpecifierPreference: "external-non-relative"
+});

Original file line number	Diff line number	Diff line change
`@@ -2927,6 +2927,10 @@ namespace FourSlash {`
`2927`	`2927`	`}`
`2928`	`2928`	`const range = ts.firstOrUndefined(ranges);`
`2929`	`2929`
	`2930`	`+ if (preferences) {`
	`2931`	`+ this.configure(preferences);`
	`2932`	`+ }`
	`2933`	`+`
`2930`	`2934`	`const codeFixes = this.getCodeFixes(fileName, errorCode, preferences).filter(f => f.fixName === ts.codefix.importFixName);`
`2931`	`2935`
`2932`	`2936`	`if (codeFixes.length === 0) {`
Original file line number	Diff line number	Diff line change
`@@ -1773,7 +1773,8 @@ namespace ts {`
`1773`	`1773`	`redirectTargetsMap: program.redirectTargetsMap,`
`1774`	`1774`	`getProjectReferenceRedirect: fileName => program.getProjectReferenceRedirect(fileName),`
`1775`	`1775`	`isSourceOfProjectReferenceRedirect: fileName => program.isSourceOfProjectReferenceRedirect(fileName),`
`1776`		`- getCompilerOptions: () => program.getCompilerOptions()`
	`1776`	`+ getCompilerOptions: () => program.getCompilerOptions(),`
	`1777`	`+ getNearestAncestorDirectoryWithPackageJson: maybeBind(host, host.getNearestAncestorDirectoryWithPackageJson),`
`1777`	`1778`	`};`
`1778`	`1779`	`}`
`1779`	`1780`
Original file line number	Diff line number	Diff line change
`@@ -610,7 +610,7 @@ declare namespace FourSlashInterface {`
`610`	`610`	`readonly includeCompletionsForModuleExports?: boolean;`
`611`	`611`	`readonly includeInsertTextCompletions?: boolean;`
`612`	`612`	`readonly includeAutomaticOptionalChainCompletions?: boolean;`
`613`		`- readonly importModuleSpecifierPreference?: "auto" \| "relative" \| "non-relative";`
	`613`	`+ readonly importModuleSpecifierPreference?: "shortest" \| "external-non-relative" \| "relative" \| "non-relative";`
`614`	`614`	`readonly importModuleSpecifierEnding?: "minimal" \| "index" \| "js";`
`615`	`615`	`}`
`616`	`616`	`interface CompletionsOptions {`