-
-
Notifications
You must be signed in to change notification settings - Fork 673
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
feat: add no-import-compiler-macros rule #2684
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for adding this rule! 🙂
'defineProps', | ||
'defineEmits', | ||
'defineExpose', | ||
'withDefaults' |
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.
Please also add defineModel
, defineOptions
and defineSlots
(and tests for them):
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.
Oh right, got it
|
||
Nothing. | ||
|
||
## :mag: Implementation |
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.
Can you please add a link to https://vuejs.org/api/sfc-script-setup.html#defineprops-defineemits in the 📚 Further Reading
section?
schema: [], | ||
messages: { | ||
noImportCompilerMacros: | ||
"'{{name}}' is a compiler macro and no longer needs to be imported from '{{source}}'." |
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.
"'{{name}}' is a compiler macro and no longer needs to be imported from '{{source}}'." | |
"'{{name}}' is a compiler macro and doesn't need to be imported." |
I think "from '{{ source }}'" sounds like it should be imported from somewhere else.
And "no longer" sounds like it had to be imported at some time, but I think it was never meant to be imported, was it?
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.
It never needs to be imported. The "no longer" stuff was directly copied from Vue's console warning..
ImportDeclaration(node) { | ||
if (node.specifiers.length === 0) return | ||
|
||
if (VUE_MODULES.has(node.source.value)) { |
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.
Better return early here, too, to remove one level of indentation and make the rest of the block easier to read:
if (node.specifiers.length === 0 || !VUE_MODULES.has(node.source.value)) {
return
}
if ( | ||
specifier.type === 'ImportSpecifier' && | ||
COMPILER_MACROS.has(specifier.imported.name) | ||
) { |
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.
Again, please return early:
if (
specifier.type !== 'ImportSpecifier' ||
!COMPILER_MACROS.has(specifier.imported.name)
) {
continue
}
const codeEnd = hasCommaAfter | ||
? tokenAfter.range[1] | ||
: specifier.range[1] |
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.
Nice that you implemented a quite elaborate auto-fix!
Can you please add test cases that these cases are handled correctly?
import { ref, defineProps } from '@vue/runtime-core'
import { defineProps as definePropsFoo } from '@vue/runtime-core'
(or maybe we ignore this case)import { ref as refFoo, defineProps, type computed } from '@vue/runtime-core'
And maybe this logic would be easier to understand?
if (isOnlySpecifier) {
removeWholeImportStatement()
} else if (isLastSpecifier) {
removeSpecifierWithPrecedingComma()
} else {
removeSpecifierWithSubsequentComma()
}
Co-authored-by: Flo Edelmann <git@flo-edelmann.de>
resolve #2437