From 45dd2ada9ba719d7fe075f46ca67fc23db5df0a7 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Fri, 3 Nov 2017 13:33:28 -0700 Subject: [PATCH 1/3] Support both baseUrl and relative paths when adding missing import --- src/services/codefixes/importFixes.ts | 134 +++++++++++------- .../importNameCodeFixNewImportBaseUrl0.ts | 3 + .../importNameCodeFixNewImportBaseUrl1.ts | 25 ++++ .../importNameCodeFixNewImportBaseUrl2.ts | 25 ++++ 4 files changed, 138 insertions(+), 49 deletions(-) create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportBaseUrl1.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportBaseUrl2.ts diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 3f0d27e5b070f..9cee680f8705b 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -206,8 +206,7 @@ namespace ts.codefix { } } } - actions.push(getCodeActionForAddImport(moduleSymbol, context, declarations)); - return actions; + return [...actions, ...getCodeActionsForAddImport(moduleSymbol, context, declarations)]; } function getNamespaceImportName(declaration: AnyImportSyntax): Identifier { @@ -296,63 +295,99 @@ namespace ts.codefix { } } - function getModuleSpecifierForNewImport(sourceFile: SourceFile, moduleSymbol: Symbol, options: CompilerOptions, getCanonicalFileName: (file: string) => string, host: LanguageServiceHost): string | undefined { + function getModuleSpecifiersForNewImport( + sourceFile: SourceFile, + moduleSymbol: Symbol, + options: CompilerOptions, + getCanonicalFileName: (file: string) => string, + host: LanguageServiceHost, + ): string[] { + const { baseUrl, paths, rootDirs } = options; const moduleFileName = moduleSymbol.valueDeclaration.getSourceFile().fileName; const sourceDirectory = getDirectoryPath(sourceFile.fileName); - 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) || - removeFileExtension(getRelativePath(moduleFileName, sourceDirectory, getCanonicalFileName)); - } + 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]; + } - function tryGetModuleNameFromAmbientModule(moduleSymbol: Symbol): string | undefined { - const decl = moduleSymbol.valueDeclaration; - if (isModuleDeclaration(decl) && isStringLiteral(decl.name)) { - return decl.name.text; + const relativePath = removeFileExtension(getRelativePath(moduleFileName, sourceDirectory, getCanonicalFileName)); + if (!baseUrl) { + return [relativePath]; + } + + const relativeToBaseUrl = getRelativePathIfInDirectory(moduleFileName, baseUrl, getCanonicalFileName); + if (!relativeToBaseUrl) { + return [relativePath]; } + + const importRelativeToBaseUrl = removeExtensionAndIndexPostFix(relativeToBaseUrl); + 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. + const relativeFirst = getRelativePathNParents(relativePath) < getRelativePathLength(getRelativePath(sourceDirectory, baseUrl, getCanonicalFileName)); + return relativeFirst ? [relativePath, importRelativeToBaseUrl] : [importRelativeToBaseUrl, relativePath]; } - function tryGetModuleNameFromBaseUrl(options: CompilerOptions, moduleFileName: string, getCanonicalFileName: (file: string) => string): string | undefined { - if (!options.baseUrl) { - return undefined; + function getRelativePathLength(relativePath: string): number { + if (startsWith(relativePath, "../")) { + return 0; + } + Debug.assert(startsWith(relativePath, "./")); + let count = 0; + for (let i = 2; i < relativePath.length; i++) { + if (relativePath.charCodeAt(i) === CharacterCodes.slash) { + count++; + } } + return count; + } - let relativeName = getRelativePathIfInDirectory(moduleFileName, options.baseUrl, getCanonicalFileName); - if (!relativeName) { - return undefined; + 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; + } - const relativeNameWithIndex = removeFileExtension(relativeName); - relativeName = removeExtensionAndIndexPostFix(relativeName); + function tryGetModuleNameFromAmbientModule(moduleSymbol: Symbol): string | undefined { + const decl = moduleSymbol.valueDeclaration; + if (isModuleDeclaration(decl) && isStringLiteral(decl.name)) { + return decl.name.text; + } + } - 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 { @@ -528,10 +563,11 @@ namespace ts.codefix { return !pathIsRelative(relativePath) ? "./" + relativePath : relativePath; } - function getCodeActionForAddImport( + function getCodeActionsForAddImport( moduleSymbol: Symbol, ctx: ImportCodeFixOptions, - declarations: ReadonlyArray): ImportCodeAction { + declarations: ReadonlyArray + ): ImportCodeAction[] { const fromExistingImport = firstDefined(declarations, declaration => { if (declaration.kind === SyntaxKind.ImportDeclaration && declaration.importClause) { const changes = tryUpdateExistingImport(ctx, declaration.importClause); @@ -547,12 +583,12 @@ namespace ts.codefix { } }); if (fromExistingImport) { - return fromExistingImport; + return [fromExistingImport]; } - const moduleSpecifier = firstDefined(declarations, moduleSpecifierFromAnyImport) - || getModuleSpecifierForNewImport(ctx.sourceFile, moduleSymbol, ctx.compilerOptions, ctx.getCanonicalFileName, ctx.host); - return getCodeActionForNewImport(ctx, moduleSpecifier); + const existingDeclaration = firstDefined(declarations, moduleSpecifierFromAnyImport); + const moduleSpecifiers = existingDeclaration ? [existingDeclaration] : getModuleSpecifiersForNewImport(ctx.sourceFile, moduleSymbol, ctx.compilerOptions, ctx.getCanonicalFileName, ctx.host); + return moduleSpecifiers.map(spec => getCodeActionForNewImport(ctx, spec)); } function moduleSpecifierFromAnyImport(node: AnyImportSyntax): string | undefined { diff --git a/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts index a48b381df68e8..bc852b5c47ce4 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 0000000000000..e690cfd3dbbf8 --- /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 0000000000000..658bd25823feb --- /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();` +]); From 8dedb637905d4ff2fa286db5e7e5e6153b72a0d2 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Fri, 10 Nov 2017 13:05:21 -0800 Subject: [PATCH 2/3] Code review --- src/services/codefixes/importFixes.ts | 35 ++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 9cee680f8705b..6b407ef994fb5 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -332,16 +332,43 @@ namespace ts.codefix { } } - // Prefer a relative import over a baseUrl import if it doesn't traverse up to baseUrl. - const relativeFirst = getRelativePathNParents(relativePath) < getRelativePathLength(getRelativePath(sourceDirectory, baseUrl, getCanonicalFileName)); + /* + 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 + baseUrlToSource = ./foo/bar + getRelativePathLength(baseUrlToSource) = 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 + baseUrlToSource = ./foo/bar + getRelativePathLength(baseUrlToSource) = 2 + 1 < 2 = true + In this case we should prefer using the relative path "../a" instead of the baseUrl path "foo/a". + */ + const pathFromBaseUrlToSource = getRelativePath(sourceDirectory, baseUrl, getCanonicalFileName); + const relativeFirst = getRelativePathNParents(relativePath) < getRelativePathLength(pathFromBaseUrlToSource); return relativeFirst ? [relativePath, importRelativeToBaseUrl] : [importRelativeToBaseUrl, relativePath]; } function getRelativePathLength(relativePath: string): number { - if (startsWith(relativePath, "../")) { + if (startsWith(relativePath, `..${directorySeparator}`)) { return 0; } - Debug.assert(startsWith(relativePath, "./")); + Debug.assert(startsWith(relativePath, `.${directorySeparator}`)); let count = 0; for (let i = 2; i < relativePath.length; i++) { if (relativePath.charCodeAt(i) === CharacterCodes.slash) { From 35aa6a04364459d396f172daec076b1ed816c642 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Tue, 21 Nov 2017 17:03:27 -0500 Subject: [PATCH 3/3] Always use getRelativePathNParents, not getRelativePathLength --- src/services/codefixes/importFixes.ts | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 6b407ef994fb5..eb18281dae8ae 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -342,8 +342,8 @@ namespace ts.codefix { Then: relativePath = ../../foo/bar getRelativePathNParents(relativePath) = 2 - baseUrlToSource = ./foo/bar - getRelativePathLength(baseUrlToSource) = 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". @@ -354,30 +354,16 @@ namespace ts.codefix { Then: relativePath = ../a getRelativePathNParents(relativePath) = 1 - baseUrlToSource = ./foo/bar - getRelativePathLength(baseUrlToSource) = 2 + 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 pathFromBaseUrlToSource = getRelativePath(sourceDirectory, baseUrl, getCanonicalFileName); - const relativeFirst = getRelativePathNParents(relativePath) < getRelativePathLength(pathFromBaseUrlToSource); + const pathFromSourceToBaseUrl = getRelativePath(baseUrl, sourceDirectory, getCanonicalFileName); + const relativeFirst = getRelativePathNParents(pathFromSourceToBaseUrl) < getRelativePathNParents(relativePath); return relativeFirst ? [relativePath, importRelativeToBaseUrl] : [importRelativeToBaseUrl, relativePath]; } - function getRelativePathLength(relativePath: string): number { - if (startsWith(relativePath, `..${directorySeparator}`)) { - return 0; - } - Debug.assert(startsWith(relativePath, `.${directorySeparator}`)); - let count = 0; - for (let i = 2; i < relativePath.length; i++) { - if (relativePath.charCodeAt(i) === CharacterCodes.slash) { - count++; - } - } - return count; - } - function getRelativePathNParents(relativePath: string): number { let count = 0; for (let i = 0; i + 3 <= relativePath.length && relativePath.slice(i, i + 3) === "../"; i += 3) {