Skip to content
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

Merged
merged 7 commits into from
Dec 5, 2023

Conversation

so1ve
Copy link
Member

@so1ve so1ve commented Dec 4, 2023

fixes #3765

so1ve added 2 commits December 4, 2023 19:25

Verified

This commit was signed with the committer’s verified signature.
so1ve Ray

Verified

This commit was signed with the committer’s verified signature.
so1ve Ray

Verified

This commit was signed with the committer’s verified signature.
so1ve Ray
@@ -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';
Copy link
Collaborator

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.

Copy link
Collaborator

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...

Copy link
Collaborator

@rchl rchl Dec 4, 2023

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.

Copy link
Member

@johnsoncodehk johnsoncodehk Dec 5, 2023

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.

Verified

This commit was signed with the committer’s verified signature.
so1ve Ray
so1ve added 2 commits December 4, 2023 20:31

Verified

This commit was signed with the committer’s verified signature.
so1ve Ray

Verified

This commit was signed with the committer’s verified signature.
so1ve Ray
@johnsoncodehk
Copy link
Member

Just had a quick look, I'll have time to test it in a few hours.

@rchl
Copy link
Collaborator

rchl commented Dec 5, 2023

I suggest to rename this in case it's to be merged because it's no longer about <component> specifically.

@johnsoncodehk johnsoncodehk changed the title fix: camelize props for <component> fix(language-core): camelize props for dynamic component Dec 5, 2023
@johnsoncodehk johnsoncodehk merged commit 613da66 into vuejs:master Dec 5, 2023
3 checks passed
@johnsoncodehk
Copy link
Member

Thanks!

so1ve added a commit to so1ve/language-tools that referenced this pull request Dec 6, 2023
Co-authored-by: Johnson Chu <johnsoncodehk@gmail.com>
xdavidwu added a commit to xdavidwu/sparkles that referenced this pull request Aug 2, 2024

Verified

This commit was signed with the committer’s verified signature.
xdavidwu Pinghao Wu
for vuejs/language-tools#3774
also unlock codemirror and adopt for new tooltip positioning behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic component shows error for props with kebab-case
3 participants