diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index dae997fc2e..4151526586 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -209,8 +209,7 @@ namespace ts.codefix { } } } - actions.push(getCodeActionForAddImport(moduleSymbols, context, declarations)); - return actions; + return [...actions, ...getCodeActionsForAddImport(moduleSymbols, context, declarations)]; } function getNamespaceImportName(declaration: AnyImportSyntax): Identifier { @@ -309,19 +308,84 @@ namespace ts.codefix { } } - export function getModuleSpecifierForNewImport(sourceFile: SourceFile, moduleSymbols: ReadonlyArray, options: CompilerOptions, getCanonicalFileName: (file: string) => string, host: LanguageServiceHost): string | undefined { - const choices = mapIterator(arrayIterator(moduleSymbols), moduleSymbol => { + export function getModuleSpecifiersForNewImport( + sourceFile: SourceFile, + moduleSymbols: ReadonlyArray, + options: CompilerOptions, + getCanonicalFileName: (file: string) => string, + host: LanguageServiceHost, + ): string[] { + const { baseUrl, paths, rootDirs } = options; + const choicesForEachExportingModule = mapIterator(arrayIterator(moduleSymbols), moduleSymbol => { const moduleFileName = moduleSymbol.valueDeclaration.getSourceFile().fileName; const sourceDirectory = getDirectoryPath(sourceFile.fileName); + const global = tryGetModuleNameFromAmbientModule(moduleSymbol) + || tryGetModuleNameFromTypeRoots(options, host, getCanonicalFileName, moduleFileName) + || tryGetModuleNameAsNodeModule(options, moduleFileName, host, getCanonicalFileName, sourceDirectory) + || rootDirs && tryGetModuleNameFromRootDirs(rootDirs, moduleFileName, sourceDirectory, getCanonicalFileName); + if (global) { + return [global]; + } - return tryGetModuleNameFromAmbientModule(moduleSymbol) || - tryGetModuleNameFromTypeRoots(options, host, getCanonicalFileName, moduleFileName) || - tryGetModuleNameAsNodeModule(options, moduleFileName, host, getCanonicalFileName, sourceDirectory) || - tryGetModuleNameFromBaseUrl(options, moduleFileName, getCanonicalFileName) || - options.rootDirs && tryGetModuleNameFromRootDirs(options.rootDirs, moduleFileName, sourceDirectory, getCanonicalFileName) || - removeExtensionAndIndexPostFix(getRelativePath(moduleFileName, sourceDirectory, getCanonicalFileName), options); + const relativePath = removeExtensionAndIndexPostFix(getRelativePath(moduleFileName, sourceDirectory, getCanonicalFileName), options); + if (!baseUrl) { + return [relativePath]; + } + + const relativeToBaseUrl = getRelativePathIfInDirectory(moduleFileName, baseUrl, getCanonicalFileName); + if (!relativeToBaseUrl) { + return [relativePath]; + } + + const importRelativeToBaseUrl = removeExtensionAndIndexPostFix(relativeToBaseUrl, options); + if (paths) { + const fromPaths = tryGetModuleNameFromPaths(removeFileExtension(relativeToBaseUrl), importRelativeToBaseUrl, paths); + if (fromPaths) { + return [fromPaths]; + } + } + + /* + 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 = getRelativePath(baseUrl, sourceDirectory, getCanonicalFileName); + const relativeFirst = getRelativePathNParents(pathFromSourceToBaseUrl) < getRelativePathNParents(relativePath); + return relativeFirst ? [relativePath, importRelativeToBaseUrl] : [importRelativeToBaseUrl, relativePath]; }); - return best(choices, (a, b) => a.length < b.length); + // Only return results for the re-export with the shortest possible path (and also give the other path even if that's long.) + return best(choicesForEachExportingModule, (a, b) => a[0].length < b[0].length); + } + + function getRelativePathNParents(relativePath: string): number { + let count = 0; + for (let i = 0; i + 3 <= relativePath.length && relativePath.slice(i, i + 3) === "../"; i += 3) { + count++; + } + return count; } function tryGetModuleNameFromAmbientModule(moduleSymbol: Symbol): string | undefined { @@ -331,44 +395,28 @@ namespace ts.codefix { } } - function tryGetModuleNameFromBaseUrl(options: CompilerOptions, moduleFileName: string, getCanonicalFileName: (file: string) => string): string | undefined { - if (!options.baseUrl) { - return undefined; - } - - let relativeName = getRelativePathIfInDirectory(moduleFileName, options.baseUrl, getCanonicalFileName); - if (!relativeName) { - return undefined; - } - - const relativeNameWithIndex = removeFileExtension(relativeName); - relativeName = removeExtensionAndIndexPostFix(relativeName, options); - - if (options.paths) { - for (const key in options.paths) { - for (const pattern of options.paths[key]) { - const indexOfStar = pattern.indexOf("*"); - if (indexOfStar === 0 && pattern.length === 1) { - continue; - } - else if (indexOfStar !== -1) { - const prefix = pattern.substr(0, indexOfStar); - const suffix = pattern.substr(indexOfStar + 1); - if (relativeName.length >= prefix.length + suffix.length && - startsWith(relativeName, prefix) && - endsWith(relativeName, suffix)) { - const matchedStar = relativeName.substr(prefix.length, relativeName.length - suffix.length); - return key.replace("\*", matchedStar); - } - } - else if (pattern === relativeName || pattern === relativeNameWithIndex) { - return key; + function tryGetModuleNameFromPaths(relativeNameWithIndex: string, relativeName: string, paths: MapLike>): string | undefined { + for (const key in paths) { + for (const pattern of paths[key]) { + const indexOfStar = pattern.indexOf("*"); + if (indexOfStar === 0 && pattern.length === 1) { + continue; + } + else if (indexOfStar !== -1) { + const prefix = pattern.substr(0, indexOfStar); + const suffix = pattern.substr(indexOfStar + 1); + if (relativeName.length >= prefix.length + suffix.length && + startsWith(relativeName, prefix) && + endsWith(relativeName, suffix)) { + const matchedStar = relativeName.substr(prefix.length, relativeName.length - suffix.length); + return key.replace("\*", matchedStar); } } + else if (pattern === relativeName || pattern === relativeNameWithIndex) { + return key; + } } } - - return relativeName; } function tryGetModuleNameFromRootDirs(rootDirs: ReadonlyArray, moduleFileName: string, sourceDirectory: string, getCanonicalFileName: (file: string) => string): string | undefined { @@ -541,10 +589,11 @@ namespace ts.codefix { return !pathIsRelative(relativePath) ? "./" + relativePath : relativePath; } - function getCodeActionForAddImport( + function getCodeActionsForAddImport( moduleSymbols: ReadonlyArray, ctx: ImportCodeFixOptions, - declarations: ReadonlyArray): ImportCodeAction { + declarations: ReadonlyArray + ): ImportCodeAction[] { const fromExistingImport = firstDefined(declarations, declaration => { if (declaration.kind === SyntaxKind.ImportDeclaration && declaration.importClause) { const changes = tryUpdateExistingImport(ctx, isImportClause(declaration.importClause) && declaration.importClause || undefined); @@ -560,12 +609,12 @@ namespace ts.codefix { } }); if (fromExistingImport) { - return fromExistingImport; + return [fromExistingImport]; } - const moduleSpecifier = firstDefined(declarations, moduleSpecifierFromAnyImport) - || getModuleSpecifierForNewImport(ctx.sourceFile, moduleSymbols, ctx.compilerOptions, ctx.getCanonicalFileName, ctx.host); - return getCodeActionForNewImport(ctx, moduleSpecifier); + const existingDeclaration = firstDefined(declarations, moduleSpecifierFromAnyImport); + const moduleSpecifiers = existingDeclaration ? [existingDeclaration] : getModuleSpecifiersForNewImport(ctx.sourceFile, moduleSymbols, ctx.compilerOptions, ctx.getCanonicalFileName, ctx.host); + return moduleSpecifiers.map(spec => getCodeActionForNewImport(ctx, spec)); } function moduleSpecifierFromAnyImport(node: AnyImportSyntax): string | undefined { diff --git a/src/services/completions.ts b/src/services/completions.ts index bfa03ef4a9..a3b94acd4c 100644 --- a/src/services/completions.ts +++ b/src/services/completions.ts @@ -488,7 +488,7 @@ namespace ts.Completions { const moduleSymbols = getAllReExportingModules(exportedSymbol, checker, allSourceFiles); Debug.assert(contains(moduleSymbols, moduleSymbol)); - const sourceDisplay = [textPart(codefix.getModuleSpecifierForNewImport(sourceFile, moduleSymbols, compilerOptions, getCanonicalFileName, host))]; + const sourceDisplay = [textPart(first(codefix.getModuleSpecifiersForNewImport(sourceFile, moduleSymbols, compilerOptions, getCanonicalFileName, host)))]; const codeActions = codefix.getCodeActionForImport(moduleSymbols, { host, checker, diff --git a/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts index a48b381df6..bc852b5c47 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts @@ -13,6 +13,9 @@ //// export function f1() { }; verify.importFixAtPosition([ +`import { f1 } from "./a/b"; + +f1();`, `import { f1 } from "b"; f1();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl1.ts b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl1.ts new file mode 100644 index 0000000000..e690cfd3db --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl1.ts @@ -0,0 +1,25 @@ +/// + +// @Filename: /tsconfig.json +////{ +//// "compilerOptions": { +//// "baseUrl": "./a" +//// } +////} + +// @Filename: /a/b/x.ts +////export function f1() { }; + +// @Filename: /a/b/y.ts +////[|f1/*0*/();|] + +goTo.file("/a/b/y.ts"); +// Order the local import first because it's simpler. +verify.importFixAtPosition([ +`import { f1 } from "./x"; + +f1();`, +`import { f1 } from "b/x"; + +f1();` +]); diff --git a/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl2.ts b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl2.ts new file mode 100644 index 0000000000..658bd25823 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl2.ts @@ -0,0 +1,25 @@ +/// + +// @Filename: /tsconfig.json +////{ +//// "compilerOptions": { +//// "baseUrl": "./a" +//// } +////} + +// @Filename: /a/b/x.ts +////export function f1() { }; + +// @Filename: /a/c/y.ts +////[|f1/*0*/();|] + +goTo.file("/a/c/y.ts"); +// Order the baseUrl import first, because the relative import climbs up to the base url. +verify.importFixAtPosition([ +`import { f1 } from "b/x"; + +f1();`, +`import { f1 } from "../b/x"; + +f1();` +]);