- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 420
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
fix(language-core): camelize props for dynamic component #3774
Conversation
@@ -1224,6 +1224,7 @@ export function generate( | |||
codes.push(`}, `); | |||
|
|||
for (const prop of props) { | |||
const shouldCamelize = !nativeTags.has(node.tag) || node.tag === 'component' || node.tag === 'Component'; |
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.
Feels hacky since the element can be called anything really. It can be a div
even.
Wouldn't it make sense to force camelize when node.tagType === CompilerDOM.ElementTypes.COMPONENT
? It appears to be a component type in this case.
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 could even dig deeper and try to figure out why component
is included in nativeTags
in the first place. But I wouldn't want to to make it too difficult in here...
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.
BTW. My initial fix (the d9351d2 commit) for those cases used a variant of the tagType
check.
@johnsoncodehk you've changed it to check against the nativeTags
without really saying why. Could you explain why you had to do that? Because at least based on the tests, the nativeTags
seems unnecessary and having a hardcoded and potentially outdated list like that feels more hacky than relying on vue compiler itself. While I understand that the Vue compiler might not provide enough information in some cases, at least it's not reflected in the tests (I think) that the nativeTags
check is necessary.
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.
nativeTags
is used in the case of using vue-language-tools for projects other than vue, e.g. uni-app projects need to specify nativeTags
to ignore the original tag names: https://github.com/uni-helper/uni-app-types#%E9%85%8D%E7%BD%AE
d9351d2 judgment based on tagType
would break this use case.
I plan to remove nativeTags
from the separate PR and suggest users to modify template compiler options via plugin api to replace nativeTags
option, so it should be possible to change to fully tagType
based judgment.
Just had a quick look, I'll have time to test it in a few hours. |
I suggest to rename this in case it's to be merged because it's no longer about |
Thanks! |
Co-authored-by: Johnson Chu <johnsoncodehk@gmail.com>
for vuejs/language-tools#3774 also unlock codemirror and adopt for new tooltip positioning behavior
fixes #3765