From 266d8de64a14cc7af2e70bc8c397fd58ff608138 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Wed, 11 Nov 2020 11:48:32 -0800 Subject: [PATCH] Proposal: importModuleSpecifierPreference: project-relative (#40637) * Add new importModuleSpecifierPreference value * Add second test * Update API baselines * Clean up and add some comments * Rename option value --- src/compiler/moduleSpecifiers.ts | 60 +++++++++++++++++-- src/compiler/types.ts | 3 +- src/harness/fourslashImpl.ts | 4 ++ src/server/editorServices.ts | 16 ++++- src/server/project.ts | 9 +++ src/server/protocol.ts | 2 +- src/services/types.ts | 2 + src/services/utilities.ts | 3 +- .../reference/api/tsserverlibrary.d.ts | 5 +- tests/baselines/reference/api/typescript.d.ts | 2 +- tests/cases/fourslash/fourslash.ts | 2 +- ...importNameCodeFix_externalNonRelateive2.ts | 44 ++++++++++++++ .../importNameCodeFix_externalNonRelative1.ts | 58 ++++++++++++++++++ 13 files changed, 197 insertions(+), 13 deletions(-) create mode 100644 tests/cases/fourslash/server/importNameCodeFix_externalNonRelateive2.ts create mode 100644 tests/cases/fourslash/server/importNameCodeFix_externalNonRelative1.ts diff --git a/src/compiler/moduleSpecifiers.ts b/src/compiler/moduleSpecifiers.ts index 8b87c5cd10225..65b047162c729 100644 --- 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( importingFileName: string, importedFileName: string, diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 0ba174ff9ea7e..8fc3a1457cd1c 100644 --- 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 index 41048899e0f0b..6af8cea9d039b 100644 --- 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 index a2fe4bd897296..7eabfc895f914 100644 --- 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 index 6e7de6af2dc5c..db704b7dfa865 100644 --- 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 index 54eddf818ff33..1f77bbfd7eb45 100644 --- 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 index 7fb1166eb19c8..bc7d7328a70ae 100644 --- 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 index bececaaa4f49c..ead7655e4692f 100644 --- 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 index 0d780a2d47770..cb10f5df5f844 100644 --- 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 index 5e9b51a30b8f6..502fb39b6971b 100644 --- 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 index fc61f9831e371..98298bbfe5f21 100644 --- 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 new file mode 100644 index 0000000000000..eb596892a3d9e --- /dev/null +++ b/tests/cases/fourslash/server/importNameCodeFix_externalNonRelateive2.ts @@ -0,0 +1,44 @@ +/// + +// @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 new file mode 100644 index 0000000000000..19c76d7ba4b4d --- /dev/null +++ b/tests/cases/fourslash/server/importNameCodeFix_externalNonRelative1.ts @@ -0,0 +1,58 @@ +/// + +// @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" +});