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.

Sign up for GitHub

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
Merged

fix(language-core): camelize props for dynamic component #3774

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
86271d3
tests
so1ve Dec 4, 2023
84f623f
fix: camelize props for `<component>`
so1ve Dec 4, 2023
0ab0dfb
move tests
so1ve Dec 4, 2023
b6eda82
refactor: use `node.tagType`
so1ve Dec 4, 2023
f777f30
minor refactor
so1ve Dec 4, 2023
e3995da
move condition
so1ve Dec 4, 2023
a0298fe
Merge branch 'master' into pr/3774
johnsoncodehk Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/language-core/src/generators/template.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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.

rchl marked this conversation as resolved.
Show resolved Hide resolved
if (
prop.type === CompilerDOM.NodeTypes.DIRECTIVE
&& (prop.name === 'bind' || prop.name === 'model')
Expand Down Expand Up @@ -1260,7 +1261,7 @@ export function generate(
if (
(!prop.arg || (prop.arg.type === CompilerDOM.NodeTypes.SIMPLE_EXPRESSION && prop.arg.isStatic)) // isStatic
&& hyphenateAttr(attrNameText) === attrNameText
&& !nativeTags.has(node.tag)
&& shouldCamelize
rchl marked this conversation as resolved.
Show resolved Hide resolved
&& !vueCompilerOptions.htmlAttributes.some(pattern => minimatch(attrNameText!, pattern))
) {
attrNameText = camelize(attrNameText);
Expand Down Expand Up @@ -1367,7 +1368,7 @@ export function generate(

if (
hyphenateAttr(prop.name) === prop.name
&& !nativeTags.has(node.tag)
&& shouldCamelize
&& !vueCompilerOptions.htmlAttributes.some(pattern => minimatch(attrNameText!, pattern))
) {
attrNameText = camelize(prop.name);
Expand Down
1 change: 1 addition & 0 deletions test-workspace/tsc/vue3_strictTemplate/dataAttributes/main.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
<path stroke="#efefef" :stroke-width="123" :pathLength="10" />
</svg>
<foo stroke-width="123" />
<component :is="Foo" stroke-width="test"></component>
</div>
</template>

Expand Down