From b8615777f3a2565628eb4c0d8b319f777b13d0b9 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Mon, 14 Dec 2020 14:26:03 -0800 Subject: [PATCH 1/2] Add failing test --- .../completionsImport_sortByDistance.ts | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 tests/cases/fourslash/completionsImport_sortByDistance.ts diff --git a/tests/cases/fourslash/completionsImport_sortByDistance.ts b/tests/cases/fourslash/completionsImport_sortByDistance.ts new file mode 100644 index 0000000000000..54e4a611ec42e --- /dev/null +++ b/tests/cases/fourslash/completionsImport_sortByDistance.ts @@ -0,0 +1,49 @@ +/// + +// @module: commonjs + +// @Filename: /src/admin/utils/db/db.ts +//// export const db = {}; + +// @Filename: /src/admin/utils/db/index.ts +//// export * from "./db"; + +// @Filename: /src/client/helpers/db.ts +//// export const db = {}; + +// @Filename: /src/client/db.ts +//// export const db = {}; + +// @Filename: /src/client/foo.ts +//// db/**/ + +verify.completions({ + marker: "", + exact: [ + completion.globalThisEntry, + ...completion.globalsVars, + completion.undefinedVarEntry, + { + name: "db", + source: "/src/client/db", + hasAction: true, + sortText: completion.SortText.AutoImportSuggestions + }, + { + name: "db", + source: "/src/client/helpers/db", + hasAction: true, + sortText: completion.SortText.AutoImportSuggestions + }, + { + name: "db", + source: "/src/admin/utils/db/index", + hasAction: true, + sortText: completion.SortText.AutoImportSuggestions + }, + ...completion.globalKeywords + ], + preferences: { + includeCompletionsForModuleExports: true + } +}); From 18d7f532ae48e322c410c47694a083803b8309aa Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Tue, 2 Feb 2021 12:13:58 -0800 Subject: [PATCH 2/2] Sort all import fixes by number of directory separators --- src/compiler/moduleSpecifiers.ts | 10 +--- src/compiler/utilities.ts | 12 +++++ src/services/codefixes/importFixes.ts | 47 +++++++++++------- .../completionsImport_sortByDistance.ts | 49 ------------------- .../importNameCodeFix_sortByDistance.ts | 26 ++++++++++ 5 files changed, 67 insertions(+), 77 deletions(-) delete mode 100644 tests/cases/fourslash/completionsImport_sortByDistance.ts create mode 100644 tests/cases/fourslash/importNameCodeFix_sortByDistance.ts diff --git a/src/compiler/moduleSpecifiers.ts b/src/compiler/moduleSpecifiers.ts index 99ccfb8c98286..e27f63eb29cda 100644 --- a/src/compiler/moduleSpecifiers.ts +++ b/src/compiler/moduleSpecifiers.ts @@ -255,16 +255,8 @@ namespace ts.moduleSpecifiers { return firstDefined(imports, ({ text }) => pathIsRelative(text) ? hasJSFileExtension(text) : undefined) || false; } - function numberOfDirectorySeparators(str: string) { - const match = str.match(/\//g); - return match ? match.length : 0; - } - function comparePathsByRedirectAndNumberOfDirectorySeparators(a: ModulePath, b: ModulePath) { - return compareBooleans(b.isRedirect, a.isRedirect) || compareValues( - numberOfDirectorySeparators(a.path), - numberOfDirectorySeparators(b.path) - ); + return compareBooleans(b.isRedirect, a.isRedirect) || compareNumberOfDirectorySeparators(a.path, b.path); } function getNearestAncestorDirectoryWithPackageJson(host: ModuleSpecifierResolutionHost, fileName: string) { diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 8d9871545ffc8..8c3ff87cbbfad 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -6586,6 +6586,18 @@ namespace ts { return false; } + function numberOfDirectorySeparators(str: string) { + const match = str.match(/\//g); + return match ? match.length : 0; + } + + export function compareNumberOfDirectorySeparators(path1: string, path2: string) { + return compareValues( + numberOfDirectorySeparators(path1), + numberOfDirectorySeparators(path2) + ); + } + /** * Extension boundaries by priority. Lower numbers indicate higher priorities, and are * aligned to the offset of the highest priority extension in the diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index e47c7f79b53cf..628c26d294e5b 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -215,7 +215,7 @@ namespace ts.codefix { : getAllReExportingModules(sourceFile, exportedSymbol, moduleSymbol, symbolName, host, program, /*useAutoImportProvider*/ true); const useRequire = shouldUseRequire(sourceFile, program); const preferTypeOnlyImport = compilerOptions.importsNotUsedAsValues === ImportsNotUsedAsValues.Error && !isSourceFileJS(sourceFile) && isValidTypeOnlyAliasUseSite(getTokenAtPosition(sourceFile, position)); - const moduleSpecifier = first(getNewImportInfos(program, sourceFile, position, preferTypeOnlyImport, useRequire, exportInfos, host, preferences)).moduleSpecifier; + const moduleSpecifier = getBestFix(getNewImportInfos(program, sourceFile, position, preferTypeOnlyImport, useRequire, exportInfos, host, preferences), sourceFile, program, host).moduleSpecifier; const fix = getImportFixForSymbol(sourceFile, exportInfos, moduleSymbol, symbolName, program, position, preferTypeOnlyImport, useRequire, host, preferences); return { moduleSpecifier, codeAction: codeFixActionToCodeAction(codeActionForFix({ host, formatContext, preferences }, sourceFile, symbolName, fix, getQuotePreference(sourceFile, preferences))) }; } @@ -223,7 +223,7 @@ namespace ts.codefix { function getImportFixForSymbol(sourceFile: SourceFile, exportInfos: readonly SymbolExportInfo[], moduleSymbol: Symbol, symbolName: string, program: Program, position: number | undefined, preferTypeOnlyImport: boolean, useRequire: boolean, host: LanguageServiceHost, preferences: UserPreferences) { Debug.assert(exportInfos.some(info => info.moduleSymbol === moduleSymbol), "Some exportInfo should match the specified moduleSymbol"); // We sort the best codefixes first, so taking `first` is best. - return first(getFixForImport(exportInfos, symbolName, position, preferTypeOnlyImport, useRequire, program, sourceFile, host, preferences)); + return getBestFix(getFixForImport(exportInfos, symbolName, position, preferTypeOnlyImport, useRequire, program, sourceFile, host, preferences), sourceFile, program, host); } function codeFixActionToCodeAction({ description, changes, commands }: CodeFixAction): CodeAction { @@ -424,28 +424,13 @@ namespace ts.codefix { ): readonly (FixAddNewImport | FixUseImportType)[] { const isJs = isSourceFileJS(sourceFile); const compilerOptions = program.getCompilerOptions(); - const { allowsImportingSpecifier } = createAutoImportFilter(sourceFile, program, host); - - const choicesForEachExportingModule = flatMap(moduleSymbols, ({ moduleSymbol, importKind, exportedSymbolIsTypeOnly }) => + return flatMap(moduleSymbols, ({ moduleSymbol, importKind, exportedSymbolIsTypeOnly }) => moduleSpecifiers.getModuleSpecifiers(moduleSymbol, program.getTypeChecker(), compilerOptions, sourceFile, createModuleSpecifierResolutionHost(program, host), preferences) .map((moduleSpecifier): FixAddNewImport | FixUseImportType => // `position` should only be undefined at a missing jsx namespace, in which case we shouldn't be looking for pure types. exportedSymbolIsTypeOnly && isJs ? { kind: ImportFixKind.ImportType, moduleSpecifier, position: Debug.checkDefined(position, "position should be defined") } : { kind: ImportFixKind.AddNew, moduleSpecifier, importKind, useRequire, typeOnly: preferTypeOnlyImport })); - - // Sort by presence in package.json, then shortest paths first - return sort(choicesForEachExportingModule, (a, b) => { - const allowsImportingA = allowsImportingSpecifier(a.moduleSpecifier); - const allowsImportingB = allowsImportingSpecifier(b.moduleSpecifier); - if (allowsImportingA && !allowsImportingB) { - return -1; - } - if (allowsImportingB && !allowsImportingA) { - return 1; - } - return a.moduleSpecifier.length - b.moduleSpecifier.length; - }); } function getFixesForAddImport( @@ -479,7 +464,31 @@ namespace ts.codefix { const info = errorCode === Diagnostics._0_refers_to_a_UMD_global_but_the_current_file_is_a_module_Consider_adding_an_import_instead.code ? getFixesInfoForUMDImport(context, symbolToken) : isIdentifier(symbolToken) ? getFixesInfoForNonUMDImport(context, symbolToken, useAutoImportProvider) : undefined; - return info && { ...info, fixes: sort(info.fixes, (a, b) => a.kind - b.kind) }; + return info && { ...info, fixes: sortFixes(info.fixes, context.sourceFile, context.program, context.host) }; + } + + function sortFixes(fixes: readonly ImportFix[], sourceFile: SourceFile, program: Program, host: LanguageServiceHost): readonly ImportFix[] { + const { allowsImportingSpecifier } = createAutoImportFilter(sourceFile, program, host); + return sort(fixes, (a, b) => compareValues(a.kind, b.kind) || compareModuleSpecifiers(a, b, allowsImportingSpecifier)); + } + + function getBestFix(fixes: readonly T[], sourceFile: SourceFile, program: Program, host: LanguageServiceHost): T { + // These will always be placed first if available, and are better than other kinds + if (fixes[0].kind === ImportFixKind.UseNamespace || fixes[0].kind === ImportFixKind.AddToExisting) { + return fixes[0]; + } + const { allowsImportingSpecifier } = createAutoImportFilter(sourceFile, program, host); + return fixes.reduce((best, fix) => + compareModuleSpecifiers(fix, best, allowsImportingSpecifier) === Comparison.LessThan ? fix : best + ); + } + + function compareModuleSpecifiers(a: ImportFix, b: ImportFix, allowsImportingSpecifier: (specifier: string) => boolean): Comparison { + if (a.kind !== ImportFixKind.UseNamespace && b.kind !== ImportFixKind.UseNamespace) { + return compareBooleans(allowsImportingSpecifier(a.moduleSpecifier), allowsImportingSpecifier(b.moduleSpecifier)) + || compareNumberOfDirectorySeparators(a.moduleSpecifier, b.moduleSpecifier); + } + return Comparison.EqualTo; } function getFixesInfoForUMDImport({ sourceFile, program, host, preferences }: CodeFixContextBase, token: Node): FixesInfo | undefined { diff --git a/tests/cases/fourslash/completionsImport_sortByDistance.ts b/tests/cases/fourslash/completionsImport_sortByDistance.ts deleted file mode 100644 index 54e4a611ec42e..0000000000000 --- a/tests/cases/fourslash/completionsImport_sortByDistance.ts +++ /dev/null @@ -1,49 +0,0 @@ -/// - -// @module: commonjs - -// @Filename: /src/admin/utils/db/db.ts -//// export const db = {}; - -// @Filename: /src/admin/utils/db/index.ts -//// export * from "./db"; - -// @Filename: /src/client/helpers/db.ts -//// export const db = {}; - -// @Filename: /src/client/db.ts -//// export const db = {}; - -// @Filename: /src/client/foo.ts -//// db/**/ - -verify.completions({ - marker: "", - exact: [ - completion.globalThisEntry, - ...completion.globalsVars, - completion.undefinedVarEntry, - { - name: "db", - source: "/src/client/db", - hasAction: true, - sortText: completion.SortText.AutoImportSuggestions - }, - { - name: "db", - source: "/src/client/helpers/db", - hasAction: true, - sortText: completion.SortText.AutoImportSuggestions - }, - { - name: "db", - source: "/src/admin/utils/db/index", - hasAction: true, - sortText: completion.SortText.AutoImportSuggestions - }, - ...completion.globalKeywords - ], - preferences: { - includeCompletionsForModuleExports: true - } -}); diff --git a/tests/cases/fourslash/importNameCodeFix_sortByDistance.ts b/tests/cases/fourslash/importNameCodeFix_sortByDistance.ts new file mode 100644 index 0000000000000..c25cf8c4d4fee --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFix_sortByDistance.ts @@ -0,0 +1,26 @@ +/// + +// @module: commonjs + +// @Filename: /src/admin/utils/db/db.ts +//// export const db = {}; + +// @Filename: /src/admin/utils/db/index.ts +//// export * from "./db"; + +// @Filename: /src/client/helpers/db.ts +//// export const db = {}; + +// @Filename: /src/client/db.ts +//// export const db = {}; + +// @Filename: /src/client/foo.ts +//// db/**/ + +goTo.marker(""); +verify.importFixAtPosition([ + `import { db } from "./db";\n\ndb`, + `import { db } from "./helpers/db";\n\ndb`, + `import { db } from "../admin/utils/db";\n\ndb`, + `import { db } from "../admin/utils/db/db";\n\ndb` +]);