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(rumtime-core): custom dom props should be cloned when cloning a hoisted DOM #3080

Merged
merged 4 commits into from
Mar 25, 2021
Merged

fix(rumtime-core): custom dom props should be cloned when cloning a hoisted DOM #3080

merged 4 commits into from
Mar 25, 2021

Conversation

HcySunYang
Copy link
Member

@HcySunYang HcySunYang commented Jan 21, 2021

close: #3072
related: #3091

@HcySunYang HcySunYang changed the title fix(rumtime-core): custom props should be cloned when cloning a hoisted DOM fix(rumtime-core): custom dom props should be cloned when cloning a hoisted DOM Jan 21, 2021
@LinusBorg LinusBorg added the ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. label Feb 4, 2021
el = vnode.el = hostCloneNode(vnode.el)
el = hostCloneNode(vnode.el)
// #3072
// should clone custom DOM props,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that, for reference, can we should make clear that this specific property is added by v-model and also used for <progress> elements.

"custom DOM props" makes it sound more generic than it is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, I think there may be more custom DOM props that need to be added in the future, not just _value, for example, for v-show's el._vod, of course, we don’t need to deal with el._vod, but it can be used as an example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah there might be others, so it's good to mention that all custom DOM props which are added by Vue should be included here - _value is one we know and can be specific about.

@LinusBorg
Copy link
Member

LinusBorg commented Feb 13, 2021

I imagine it's pretty hard to write a proper test for this, but can you think of a way to test this?

Would like to ship this with the next patch.

@HcySunYang
Copy link
Member Author

I imagine it's pretty hard to write a proper test for this, but can you think of a way to test this?
Would like to ship this with the next patch.

yeah, let me try

@HcySunYang
Copy link
Member Author

I changed the implementation

@yyx990803 yyx990803 merged commit 5dbe834 into vuejs:master Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data binding does not update for transpiled app
3 participants