Skip to content

Fixed an issue with type-only import promoting #55365

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
Sep 18, 2023
Merged

Fixed an issue with type-only import promoting #55365

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
e0c8912
Fixed an issue with type-only import promoting
Andarist Aug 14, 2023
d3dc8aa
Merge remote-tracking branch 'origin/main' into fix/codefix-promote-t…
Andarist Aug 16, 2023
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
11 changes: 6 additions & 5 deletions src/services/codefixes/importFixes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1362,15 +1362,16 @@ function promoteFromTypeOnly(changes: textChanges.ChangeTracker, aliasDeclaratio
if (aliasDeclaration.isTypeOnly) {
const sortKind = OrganizeImports.detectImportSpecifierSorting(aliasDeclaration.parent.elements, preferences);
if (aliasDeclaration.parent.elements.length > 1 && sortKind) {
changes.delete(sourceFile, aliasDeclaration);
const newSpecifier = factory.updateImportSpecifier(aliasDeclaration, /*isTypeOnly*/ false, aliasDeclaration.propertyName, aliasDeclaration.name);
const comparer = OrganizeImports.getOrganizeImportsComparer(preferences, sortKind === SortKind.CaseInsensitive);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that there is an argument to be made here that this codefix shouldn't change the position of the promoted import. This is a separate action. I left this as-is though, as I assume that this was implemented this way for a reason.

Copy link

Choose a reason for hiding this comment

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

I suppose it might be annoying to users if they have their imports already sorted such that type-only imports are grouped separately from regular ones; promoting a type import will leave it in the wrong position in the list. Particularly if the user is relying on auto-imports which automatically puts them in the correct place, it'd be frustrating to have this one code fix that requires triggering Organize Imports manually when in every other case it's automatic.

Copy link
Member

Choose a reason for hiding this comment

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

The reason that it moves in the list currently is that TS's organize/auto-imports always sorts type imports at end, but otherwise they're in order. Of course, via #55269 and #52820, this will become even more configurable. So effectively, any operation that does anything to imports (adding, removing, changing modifiers) has to go through some sort of sorting code to determine whether or not the user opted into sorting something in a particular way. Thankfully, the main goal of that configurability is enabling people to stick type imports inline such that these modifier changes don't affect sorting at all, which is nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So effectively, any operation that does anything to imports (adding, removing, changing modifiers) has to go through some sort of sorting code to determine whether or not the user opted into sorting something in a particular way.

Do you happen to know if renaming exported symbols also sorts the relevant imports that use them?

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure it doesn't, no; my understanding is that rename is more of an "in place transform" and it doesn't even check that the name you're giving it is syntactically valid.

const insertionIndex = OrganizeImports.getImportSpecifierInsertionIndex(aliasDeclaration.parent.elements, newSpecifier, comparer);
changes.insertImportSpecifierAtIndex(sourceFile, newSpecifier, aliasDeclaration.parent, insertionIndex);
}
else {
changes.deleteRange(sourceFile, aliasDeclaration.getFirstToken()!);
if (aliasDeclaration.parent.elements.indexOf(aliasDeclaration) !== insertionIndex) {
changes.delete(sourceFile, aliasDeclaration);
changes.insertImportSpecifierAtIndex(sourceFile, newSpecifier, aliasDeclaration.parent, insertionIndex);
return aliasDeclaration;
}
}
changes.deleteRange(sourceFile, aliasDeclaration.getFirstToken()!);
return aliasDeclaration;
}
else {
Expand Down
21 changes: 21 additions & 0 deletions tests/cases/fourslash/codeFixPromoteTypeOnly1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/// <reference path="fourslash.ts" />
// @module: es2015

// https://github.com/microsoft/TypeScript/issues/55363

// @Filename: index.ts
//// import { TwistyAlgEditor, type TwistyPlayer } from "./other-file";
//// new TwistyPlayer();

// @Filename: other-file.ts
//// export class TwistyAlgEditor {}
//// export class TwistyPlayer {}

verify.codeFix({
index: 0,
description: [ts.Diagnostics.Remove_type_from_import_of_0_from_1.message, "TwistyPlayer", "./other-file"],
applyChanges: true,
newFileContent:
`import { TwistyAlgEditor, TwistyPlayer } from "./other-file";
new TwistyPlayer();`
})