-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Add refactoring to use default import #19659
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
44919e8
to
6d49aeb
Compare
|
||
const useDefaultImport: Refactor = { | ||
name: actionName, | ||
description: actionName, |
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.
this should be localizable, e.g. getLocaleSpecificMessage(Diagnostics.Convert_to_default_import),
name should be not be modified, it should act as an identifier
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.
If the locale is changed after boot this will have the wrong description, since this object is created immediately. This would affect other refactors too. Should we fix this?
function getAvailableActions(context: RefactorContext): ApplicableRefactorInfo[] | undefined { | ||
const { file, startPosition, program } = context; | ||
|
||
if (!program.getCompilerOptions().allowSyntheticDefaultImports) { |
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.
@weswigham we will need to add your new commandline argument to enable the interop behavior here as well.
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.
If you go look at the PR, I have a similar but more precise quickfix which triggers on a diagnostic added when a namespace object is misused.
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 think we need both. The quick fix for the error, the refactoring for style change even without an error.
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.
We need another one for .js files to convert require
to either import d from "mod"
or import * from "mod"
based on the shape of the module
@mhegazy Could you clarify what you mean by "based on the shape of the module"? When should we use a namespace import instead of a default import? |
Logged #19719 to track that |
Fixes #19261
Currently this will only activate if
--allowSyntheticDefaultImports
is enabled.