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(runtime-core): custom-element: ensure event names are hyphenated (fix: #5373) #5378

Merged
merged 2 commits into from Nov 11, 2022

Conversation

LinusBorg
Copy link
Member

close: #5373

@netlify
Copy link

netlify bot commented Feb 8, 2022

✔️ Deploy Preview for vuejs-coverage ready!

🔨 Explore the source changes: 3d55f46

🔍 Inspect the deploy log: https://app.netlify.com/sites/vuejs-coverage/deploys/6202a3cbf0d99a0008d3aa6a

😎 Browse the preview: https://deploy-preview-5378--vuejs-coverage.netlify.app

@netlify
Copy link

netlify bot commented Feb 8, 2022

✔️ Deploy Preview for vue-sfc-playground ready!

🔨 Explore the source changes: 3d55f46

🔍 Inspect the deploy log: https://app.netlify.com/sites/vue-sfc-playground/deploys/6202a3cba98bb00008bd215c

😎 Browse the preview: https://deploy-preview-5378--vue-sfc-playground.netlify.app/

@netlify
Copy link

netlify bot commented Feb 8, 2022

✔️ Deploy Preview for vue-next-template-explorer ready!

🔨 Explore the source changes: 3d55f46

🔍 Inspect the deploy log: https://app.netlify.com/sites/vue-next-template-explorer/deploys/6202a3cb77f1480007536f2d

😎 Browse the preview: https://deploy-preview-5378--vue-next-template-explorer.netlify.app

@LinusBorg LinusBorg added 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. feat: custom elements labels Feb 8, 2022
@LinusBorg LinusBorg added the 🔍 review needed This PR requires a review by a member label Jun 25, 2022
@LinusBorg
Copy link
Member Author

LinusBorg commented Jun 25, 2022

My current implementation would be a breaking change as it would break camelCase listeners that were added programmatically, for example.

Could we get around this by emitting the event both in camel- and kebap-case?

That could also break something, but I'd consider that to be the most extreme of edge cases.

@LinusBorg LinusBorg moved this from PRs to Review needed in Custom Elements Bugsquash Jun 25, 2022
@yyx990803 yyx990803 force-pushed the linusborg/ce-hyphenate-events-5373 branch from 3d55f46 to b7b934d Compare Nov 11, 2022
@yyx990803
Copy link
Member

Could we get around this by emitting the event both in camel- and kebap-case?
That could also break something, but I'd consider that to be the most extreme of edge cases.

I think this is safer than always hyphenating. Technically, I don't think custom elements should expect case conversions for emitted events. But the mismatch between Vue conventions is also a valid issue, so I think emitting both versions is an ok compromise.

@yyx990803 yyx990803 merged commit 0b39e46 into main Nov 11, 2022
13 of 14 checks passed
Custom Elements Bugsquash automation moved this from Review needed to Done Nov 11, 2022
@yyx990803 yyx990803 deleted the linusborg/ce-hyphenate-events-5373 branch Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: custom elements 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. 🔍 review needed This PR requires a review by a member
Development

Successfully merging this pull request may close these issues.

Event names for Custom Elements should be automatically transformed
4 participants