-
Notifications
You must be signed in to change notification settings - Fork 395
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: prevent prematurely cloning light dom slot vnodes #4258
Conversation
} else { | ||
// vnode.elm is undefined during initial render. | ||
// We don't need to clone at this point because it doesn't need to be unmounted. | ||
vnode.slotAssignment = data.slotAssignment; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main issue was that we were cloning on every opportunity, but when vnode.elm
is undefined
, we miss attaching the element to the vnode entirely which causes a cascade of downstream effects.
Cloning should only be done for slot forwarding which is safe because the previous vnodes will be unmounted and discarded immediately as part of vdom diffing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the explanation for why we update the slotAssignment here instead? I'm not very familiar with that property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This runs when the light dom slot forwards its slot assignment to the slottee.
For example:
<x-light-forwarded-slot>
<div slot="forwardedSlot"></div>
</x-light-forwarded-slot>
<x-light-named-slot>
<slot name="forwardedSlot" slot="namedSlot"></slot>
</x-light-named-slot>
<!--- x-light-forwarded-slot --->
<template>
<slot name="namedSlot"></slot>
</template>
<!--- x-light-named-slot --->
When the vnode reaches this point in the code, the div
's vnode.slotAssignment
will be forwardedSlot
, we need to update it to reflect namedSlot
so that it is slotted into the correct position in x-light-named-slot
.
We need to update the slotAssignment
here because we need to get the latest slot assignment so the content is slotted into the correct position.
@@ -279,7 +279,15 @@ function s( | |||
// to the vnode because the current way the diffing algo works, it will replace the original reference | |||
// to the host element with a new one. This means the new element will be mounted and immediately unmounted. | |||
// Creating a copy of the vnode to preserve a reference to the previous host element. | |||
clonedVNode = { ...vnode, slotAssignment: data.slotAssignment }; | |||
if (!isUndefined(vnode.elm)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to reverse the logic here. if (!foo) {} else {}
is always harder to read than if (foo) else {}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated! - 371a420
window.timingBuffer = []; | ||
}; | ||
|
||
if (USE_LIGHT_DOM_SLOT_FORWARDING) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this just not work without light DOM slot forwarding? I thought this was a regression, so it worked with slot forwarding turned off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, this tests that slot forwarding works correctly.
I would add another, simpler test to confirm that disconnectedCalback
fires both with and without slot forwarding. This will confirm the fix from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a simple test for the standard slot scenario! - 998a496
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed the fix – without your PR, the standard slotting
test fails in the latest API version but passes in API_VERSION=60
.
Details
The fix introduced in #3883 resulted in a regression where slotted elements inside a light dom slot were not invoking
disconnectedCallback
.For example:
When
<x-light-named-slot>
is removed from the DOM,<x-child>
'sdisconnectedCallback
is never invoked.The issue is caused by prematurely cloning light dom slot vnodes when the
vnode.elm
isundefined
.The reason cloning at this point is premature is that cloning is only necessary during slot forwarding, when the
slot
attribute is dynamic.The reason it is safe to clone the node at this point is that the vnode being cloned will be unmounted and discarded immediately following the vdom diffing completion.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
disconnectedCallback
will now work for custom elements slotted in a light dom slot.GUS work item
W-15833067