Fix ordering of module specifiers based on package.json presence #46437
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
| 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)) |
There was a problem hiding this comment.
From compareBooleans:
trueis greater thanfalse
So we need to reverse the order here to ensure module specifiers that are allowed are sorted LessThan module specifiers that aren’t.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| 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 thisThe 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
thanks for explaining your thought process! 😊
| 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)) |
There was a problem hiding this comment.
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?
…rosoft#46437) * Add failing test * Fix ordering of module specifiers based on package.json presence
Fixes #46332
Guess I got
Comparison.GreaterThanandComparison.LessThanmixed up 9 months ago in #42614 😵