-
Notifications
You must be signed in to change notification settings - Fork 13.1k
Code fix should prefer import type
#54255
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.
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
Conversation
import type
|
|
||
| if (some(importDeclarationChanges)) { | ||
| return [ | ||
| createCodeFixActionWithoutFixAll(fixId, importDeclarationChanges, Diagnostics.Use_import_type), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure if the two fix options should have different fixNames
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good to me but i don't know about this either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asked at the editor sync and this is only used for telemetry, and the telemetry stakeholders don’t care if they have the same name.
jakebailey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though it seems like our Convert ... language is somewhat universal; is it worth preserving that wording?
|
I think it’s usually better to quote exact syntax, and “type-only import” is ambiguous between type-only import declarations and type-only import specifiers. Not sure how to preserve the word “convert” and disambiguate, and I also don’t think I like the word to describe just adding a modifier. |
|
I’m going to add a fix-all test before merging. |
Noticed this while reviewing @johnnyreilly’s post about
importsNotUsedAsValues,verbatimModuleSyntax, and eslint.importsNotUsedAsValueswould produce errors any time the whole import declaration wasn’t marked as type-only but could be, so the error span was over the whole import, and the only fix that would satisfy it was to use a top-levelimport type.verbatimModuleSyntax, on the other hand, cares more about whether the individual import specifiers are safe to emit, so the error span from that option is placed on individual import specifiers, and using atypemodifier on each specifier can satisfy the check. However, underverbatimModuleSyntax, it’s generally preferable to use a top-levelimport typeon the whole import declaration when possible, and the built-in codefix hadn’t been updated to consider that. Even though it’s no longer a TS error to have an import that will unnecessarily be emitted to JS, default behavior from auto-imports and codefixes should avoid that.For individual fixes, both fix options are offered if
import typeis possible, with theimport typebeing sorted first:Using
import typeis not possible when other specifiers are already used as values:For fix-all,
import typeis used whenever possible.