Skip to content

Fix ordering of module specifiers based on package.json presence #46437

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 21, 2021
Merged

Fix ordering of module specifiers based on package.json presence #46437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
984d612
Add failing test
andrewbranch Oct 15, 2021
2cc0090
Fix ordering of module specifiers based on package.json presence
andrewbranch Oct 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/services/codefixes/importFixes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,13 +663,15 @@ namespace ts.codefix {
}
const { allowsImportingSpecifier } = createPackageJsonImportFilter(sourceFile, preferences, host);
return fixes.reduce((best, fix) =>
// Takes true branch of conditional if `fix` is better than `best`
compareModuleSpecifiers(fix, best, sourceFile, program, allowsImportingSpecifier) === Comparison.LessThan ? fix : best
);
}

/** @returns `Comparison.LessThan` if `a` is better than `b`. */
function compareModuleSpecifiers(a: ImportFix, b: ImportFix, importingFile: SourceFile, program: Program, allowsImportingSpecifier: (specifier: string) => boolean): Comparison {
if (a.kind !== ImportFixKind.UseNamespace && b.kind !== ImportFixKind.UseNamespace) {
return compareBooleans(allowsImportingSpecifier(a.moduleSpecifier), allowsImportingSpecifier(b.moduleSpecifier))
return compareBooleans(allowsImportingSpecifier(b.moduleSpecifier), allowsImportingSpecifier(a.moduleSpecifier))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From compareBooleans:

true is greater than false

So we need to reverse the order here to ensure module specifiers that are allowed are sorted LessThan module specifiers that aren’t.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for my own understanding: is there a reason why you chose to reverse a and b here instead of, say, changing from Comparison.LessThan to Comparison.GreaterThan?

Copy link
Member Author

@andrewbranch andrewbranch Oct 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was simplest to leave the sort order going from LessThan to GreaterThan (just like numbers are sorted), so I think the simplest two options were this and

Suggested change
return compareBooleans(allowsImportingSpecifier(b.moduleSpecifier), allowsImportingSpecifier(a.moduleSpecifier))
return compareBooleans(!allowsImportingSpecifier(a.moduleSpecifier), !allowsImportingSpecifier(b.moduleSpecifier))

which would be read like “true is greater than false → doesn’t allow importing this specifier is greater than allows importing this specifier → ones with disallowed specifiers are sorted last.” That might be more direct reading from zero context, but when I realized I just needed to reverse the polarity of this comparison, swapping the order of the comparison was my go-to, probably because of the common analog of

nums.sort((a, b) => a - b); // how do you reverse this sort order?
nums.sort((a, b) => b - a); // like this

The other option that occurred to me was multiplying the comparison by -1, but I felt like that defeated the semi-opaque quality of the Comparison enum.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultimately, this whole comparison abstraction makes it really nice to read at a high level what factors are considered in sorting/comparing, but very confusing to map onto the low level of what .reduce or .sort is going to do. Good for understanding the big picture, bad for fixing bugs like this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for explaining your thought process! 😊

|| compareNodeCoreModuleSpecifiers(a.moduleSpecifier, b.moduleSpecifier, importingFile, program)
|| compareNumberOfDirectorySeparators(a.moduleSpecifier, b.moduleSpecifier);
}
Expand Down
93 changes: 93 additions & 0 deletions tests/cases/fourslash/completionsImport_46332.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/// <reference path="fourslash.ts" />

// @module: esnext
// @moduleResolution: node

// @Filename: /node_modules/vue/package.json
//// {
//// "name": "vue",
//// "types": "dist/vue.d.ts"
//// }

// @Filename: /node_modules/vue/dist/vue.d.ts
//// export * from "@vue/runtime-dom"

// @Filename: /node_modules/@vue/runtime-dom/package.json
//// {
//// "name": "@vue/runtime-dom",
//// "types": "dist/runtime-dom.d.ts"
//// }

// @Filename: /node_modules/@vue/runtime-dom/dist/runtime-dom.d.ts
//// export * from "@vue/runtime-core";
//// export {}
//// declare module '@vue/reactivity' {
//// export interface RefUnwrapBailTypes {
//// runtimeDOMBailTypes: any
//// }
//// }

// @Filename: /node_modules/@vue/runtime-core/package.json
//// {
//// "name": "@vue/runtime-core",
//// "types": "dist/runtime-core.d.ts"
//// }

// @Filename: /node_modules/@vue/runtime-core/dist/runtime-core.d.ts
//// import { ref } from '@vue/reactivity';
//// export { ref };
//// declare module '@vue/reactivity' {
//// export interface RefUnwrapBailTypes {
//// runtimeCoreBailTypes: any
//// }
//// }

// @Filename: /node_modules/@vue/reactivity/package.json
//// {
//// "name": "@vue/reactivity",
//// "types": "dist/reactivity.d.ts"
//// }

// @Filename: /node_modules/@vue/reactivity/dist/reactivity.d.ts
//// export declare function ref<T = any>(): T;

// @Filename: /package.json
//// {
//// "dependencies": {
//// "vue": "*"
//// }
//// }

// @Filename: /index.ts
//// import {} from "vue";
//// ref/**/

verify.completions({
marker: "",
includes: [{
name: "ref",
source: "vue",
sourceDisplay: "vue",
hasAction: true,
sortText: completion.SortText.AutoImportSuggestions,
}],
preferences: {
includeCompletionsForModuleExports: true,
allowIncompleteCompletions: true,
},
});

verify.applyCodeActionFromCompletion("", {
name: "ref",
source: "vue",
description: `Add 'ref' to existing import declaration from "vue"`,
data: {
exportName: "ref",
fileName: "/node_modules/vue/dist/vue.d.ts",
},
preferences: {
includeCompletionsForModuleExports: true,
allowIncompleteCompletions: true,
},
newFileContent: `import { ref } from "vue";\nref`
});