Proposal: importModuleSpecifierPreference: project-relative (microsoft#40637)

andrewbranch · web-flow · commit 266d8de64a14 · 2020-11-11T11:48:32.000-08:00
* Add new importModuleSpecifierPreference value

* Add second test

* Update API baselines

* Clean up and add some comments

* Rename option value
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 === "project-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);
@@ -183,7 +189,42 @@ namespace ts.moduleSpecifiers {
             return nonRelative;
         }
 
-        if (relativePreference !== RelativePreference.Auto) Debug.assertNever(relativePreference);
+        if (relativePreference === RelativePreference.ExternalNonRelative) {
+            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.
+                //
+                //      src/
+                //        tsconfig.json
+                //        index.ts -------
+                //      lib/              | (path crosses tsconfig.json)
+                //        imported.ts <---
+                //
+                return nonRelative;
+            }
+
+            const nearestTargetPackageJson = getNearestAncestorDirectoryWithPackageJson(host, getDirectoryPath(modulePath));
+            const nearestSourcePackageJson = getNearestAncestorDirectoryWithPackageJson(host, sourceDirectory);
+            if (nearestSourcePackageJson !== nearestTargetPackageJson) {
+                // 2. The importing and imported files are part of different packages.
+                //
+                //      packages/a/
+                //        package.json
+                //        index.ts --------
+                //      packages/b/        | (path crosses package.json)
+                //        package.json     |
+                //        component.ts <---
+                //
+                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;
@@ -213,6 +254,15 @@ namespace ts.moduleSpecifiers {
         );
     }
 
+    function getNearestAncestorDirectoryWithPackageJson(host: ModuleSpecifierResolutionHost, fileName: string) {
+        if (host.getNearestAncestorDirectoryWithPackageJson) {
+            return host.getNearestAncestorDirectoryWithPackageJson(fileName);
+        }
+        return !!forEachAncestorDirectory(fileName, directory => {
+            return host.fileExists(combinePaths(directory, "package.json")) ? true : undefined;
+        });
+    }
+
     export function forEachFileNameOfModule<T>(
         importingFileName: string,
         importedFileName: string,
diff --git a/src/compiler/types.ts b/src/compiler/types.ts
@@ -7858,6 +7858,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;
@@ -8159,7 +8160,7 @@ namespace ts {
         readonly includeCompletionsForModuleExports?: boolean;
         readonly includeAutomaticOptionalChainCompletions?: boolean;
         readonly includeCompletionsWithInsertText?: boolean;
-        readonly importModuleSpecifierPreference?: "auto" | "relative" | "non-relative";
+        readonly importModuleSpecifierPreference?: "shortest" | "project-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
@@ -2937,6 +2937,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
@@ -3878,7 +3878,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;
                 }
             };
@@ -3887,6 +3887,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
@@ -1651,6 +1651,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);
@@ -1994,6 +1999,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
@@ -3231,7 +3231,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" | "project-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): string | undefined;
+        /* @internal */
         getPackageJsonsForAutoImport?(rootDir?: string): readonly PackageJsonInfo[];
         /* @internal */
         getImportSuggestionsCache?(): Completions.ImportSuggestionsForFileCache;
diff --git a/src/services/utilities.ts b/src/services/utilities.ts
@@ -1808,7 +1808,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/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts
@@ -3866,7 +3866,7 @@ declare namespace ts {
         readonly includeCompletionsForModuleExports?: boolean;
         readonly includeAutomaticOptionalChainCompletions?: boolean;
         readonly includeCompletionsWithInsertText?: boolean;
-        readonly importModuleSpecifierPreference?: "auto" | "relative" | "non-relative";
+        readonly importModuleSpecifierPreference?: "shortest" | "project-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;
@@ -9004,7 +9004,7 @@ declare 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" | "project-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;
@@ -9387,6 +9387,7 @@ declare namespace ts.server {
         private rootFileNames;
         isOrphan(): boolean;
         updateGraph(): boolean;
+        hasRoots(): boolean;
         markAsDirty(): void;
         getScriptFileNames(): string[];
         getLanguageService(): never;
diff --git a/tests/baselines/reference/api/typescript.d.ts b/tests/baselines/reference/api/typescript.d.ts
@@ -3866,7 +3866,7 @@ declare namespace ts {
         readonly includeCompletionsForModuleExports?: boolean;
         readonly includeAutomaticOptionalChainCompletions?: boolean;
         readonly includeCompletionsWithInsertText?: boolean;
-        readonly importModuleSpecifierPreference?: "auto" | "relative" | "non-relative";
+        readonly importModuleSpecifierPreference?: "shortest" | "project-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/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" | "project-relative" | "relative" | "non-relative";
         readonly importModuleSpecifierEnding?: "minimal" | "index" | "js";
     }
     interface CompletionsOptions {
diff --git a/tests/cases/fourslash/server/importNameCodeFix_externalNonRelateive2.ts b/tests/cases/fourslash/server/importNameCodeFix_externalNonRelateive2.ts
@@ -0,0 +1,44 @@
+/// <reference path="../fourslash.ts" />
+
+// @Filename: /apps/app1/tsconfig.json
+//// {
+////   "compilerOptions": {
+////     "module": "commonjs",
+////     "paths": {
+////       "shared/*": ["../../shared/*"]
+////     }
+////   },
+////   "include": ["src", "../../shared"]
+//// }
+
+// @Filename: /apps/app1/src/index.ts
+//// shared/*internal2external*/
+
+// @Filename: /apps/app1/src/app.ts
+//// utils/*internal2internal*/
+
+// @Filename: /apps/app1/src/utils.ts
+//// export const utils = 0;
+
+// @Filename: /shared/constants.ts
+//// export const shared = 0;
+
+// @Filename: /shared/data.ts
+//// shared/*external2external*/
+
+format.setOption("newline", "\n");
+
+goTo.marker("internal2external");
+verify.importFixAtPosition([`import { shared } from "shared/constants";\n\nshared`], /*errorCode*/ undefined, {
+  importModuleSpecifierPreference: "project-relative"
+});
+
+goTo.marker("internal2internal");
+verify.importFixAtPosition([`import { utils } from "./utils";\n\nutils`], /*errorCode*/ undefined, {
+  importModuleSpecifierPreference: "project-relative"
+});
+
+goTo.marker("external2external");
+verify.importFixAtPosition([`import { shared } from "./constants";\n\nshared`], /*errorCode*/ undefined, {
+  importModuleSpecifierPreference: "project-relative"
+});
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: "project-relative"
+});
+
+goTo.marker("internal");
+verify.importFixAtPosition([`import { Pkg2 } from "../../utils";\n\nPkg2`], /*errorCode*/ undefined, {
+  importModuleSpecifierPreference: "project-relative"
+});

Original file line number	Diff line number	Diff line change
`@@ -2937,6 +2937,10 @@ namespace FourSlash {`
`2937`	`2937`	`}`
`2938`	`2938`	`const range = ts.firstOrUndefined(ranges);`
`2939`	`2939`
	`2940`	`+ if (preferences) {`
	`2941`	`+ this.configure(preferences);`
	`2942`	`+ }`
	`2943`	`+`
`2940`	`2944`	`const codeFixes = this.getCodeFixes(fileName, errorCode, preferences).filter(f => f.fixName === ts.codefix.importFixName);`
`2941`	`2945`
`2942`	`2946`	`if (codeFixes.length === 0) {`
Original file line number	Diff line number	Diff line change
`@@ -1808,7 +1808,8 @@ namespace ts {`
`1808`	`1808`	`redirectTargetsMap: program.redirectTargetsMap,`
`1809`	`1809`	`getProjectReferenceRedirect: fileName => program.getProjectReferenceRedirect(fileName),`
`1810`	`1810`	`isSourceOfProjectReferenceRedirect: fileName => program.isSourceOfProjectReferenceRedirect(fileName),`
`1811`		`- getCompilerOptions: () => program.getCompilerOptions()`
	`1811`	`+ getCompilerOptions: () => program.getCompilerOptions(),`
	`1812`	`+ getNearestAncestorDirectoryWithPackageJson: maybeBind(host, host.getNearestAncestorDirectoryWithPackageJson),`
`1812`	`1813`	`};`
`1813`	`1814`	`}`
`1814`	`1815`