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: ensure customElements have set domprop listeners synchronously if possible. #5328

Merged
merged 2 commits into from Nov 11, 2022

Conversation

LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Jan 25, 2022

Problem

defineCustomElement reflects all props as element properties, but it does so only after the element has connected.

When Vue sets props on elements, it prefers to add them via element properties if if can find a matching property on the element.

But since defineCustomElement sets these property getter/setters only after the element has connected, they don't exist when Vue is looking for them during mount.

This means that we can't do this:

<my-el  :object-prop="{ some: 'object' }" />

because it will end up as an attribute containing [object Object].

We have to do use the .prop modifier:

<my-el  :object-prop.prop="{ some: 'object' }" />

...but not all web frameworks support such a feature.

Solution

This PR is a refactoring that ensures that the element property getter/setters are set synchronously in the constructor in order to solve this problem.

Limitations

This solution doesn't work for async cusotm elements, as we can only resolve the props after the component definition has been loaded asynchronously.

close: #2343

@netlify
Copy link

netlify bot commented Jan 25, 2022

✔️ Deploy Preview for vuejs-coverage ready!

🔨 Explore the source changes: 6dff8a2

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

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

@netlify
Copy link

netlify bot commented Jan 25, 2022

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

🔨 Explore the source changes: 6dff8a2

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

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

@netlify
Copy link

netlify bot commented Jan 25, 2022

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

🔨 Explore the source changes: 6dff8a2

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

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

@LinusBorg LinusBorg force-pushed the linusborg/customel-props-preconnect branch from a2aa5fb to 6dff8a2 Compare Jan 25, 2022
@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 🔍 review needed This PR requires a review by a member labels Jan 25, 2022
@LinusBorg LinusBorg added this to Review needed in Custom Elements Bugsquash Jun 25, 2022
@LinusBorg LinusBorg marked this pull request as draft Jun 25, 2022
@LinusBorg
Copy link
Member Author

Converted to draft so it doesn't get merged as of now. This should be merged after a lot of other smaller fixes have been added, at which point this PR here will require some adjustments for the expected merge conflicts.

@LinusBorg LinusBorg moved this from Review needed to PRs in Custom Elements Bugsquash Jun 25, 2022
@yyx990803 yyx990803 force-pushed the linusborg/customel-props-preconnect branch from 6dff8a2 to 19c8aa8 Compare Nov 11, 2022
@yyx990803 yyx990803 marked this pull request as ready for review Nov 11, 2022
@yyx990803
Copy link
Member

yyx990803 commented Nov 11, 2022

Note: this fix mainly allows users to do <my-el :object-prop="{ some: 'object' }" /> inside a Vue app when consuming Vue-defined custom elements. It also helps when used in other frameworks that detect prop setting via property presence check (e.g. Preact). It does not fix such a case for 3rd party web components that do not define properties before connect, so technically related to #2343 but not a fix for it.

@yyx990803 yyx990803 merged commit 55382ae into main Nov 11, 2022
13 of 14 checks passed
Custom Elements Bugsquash automation moved this from PRs to Done Nov 11, 2022
@yyx990803 yyx990803 deleted the linusborg/customel-props-preconnect 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.

Not able to pass object as attr with stencil.js generated web component
2 participants