-
Notifications
You must be signed in to change notification settings - Fork 393
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
Changes from all commits
a9ede3e
5867d15
ddbfbcc
371a420
998a496
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,172 @@ | ||
import { createElement } from 'lwc'; | ||
import { extractDataIds, USE_LIGHT_DOM_SLOT_FORWARDING } from 'test-utils'; | ||
|
||
import SlotForwarding from 'x/slotForwarding'; | ||
import DynamicSlotForwarding from 'x/dynamicSlotForwarding'; | ||
import StandardSlotting from 'x/standardSlotting'; | ||
|
||
import { resetId } from './util.js'; | ||
|
||
const resetTimingBuffer = () => { | ||
window.timingBuffer = []; | ||
}; | ||
|
||
describe('standard slotting', () => { | ||
beforeEach(() => { | ||
window.timingBuffer = []; | ||
resetId(); | ||
}); | ||
|
||
afterEach(() => { | ||
delete window.timingBuffer; | ||
}); | ||
|
||
it('invokes lifecycle methods in correct order', async () => { | ||
const elm = createElement('x-standard-slotting', { is: StandardSlotting }); | ||
elm.show = true; | ||
document.body.appendChild(elm); | ||
await Promise.resolve(); | ||
|
||
expect(window.timingBuffer).toEqual([ | ||
'0:connectedCallback', | ||
'1:connectedCallback', | ||
'2:connectedCallback', | ||
]); | ||
|
||
resetTimingBuffer(); | ||
|
||
elm.show = false; | ||
await Promise.resolve(); | ||
|
||
expect(window.timingBuffer).toEqual([ | ||
'0:disconnectedCallback', | ||
'1:disconnectedCallback', | ||
'2:disconnectedCallback', | ||
]); | ||
}); | ||
}); | ||
|
||
if (USE_LIGHT_DOM_SLOT_FORWARDING) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed the fix – without your PR, the |
||
describe('slot forwarding', () => { | ||
beforeEach(() => { | ||
window.timingBuffer = []; | ||
resetId(); | ||
}); | ||
|
||
afterEach(() => { | ||
delete window.timingBuffer; | ||
}); | ||
|
||
it('invokes lifecycle methods in correct order - static', async () => { | ||
const elm = createElement('x-slot-forwarding', { is: SlotForwarding }); | ||
document.body.appendChild(elm); | ||
await Promise.resolve(); | ||
|
||
expect(window.timingBuffer).toEqual([ | ||
'0:connectedCallback', | ||
'1:connectedCallback', | ||
'2:connectedCallback', | ||
]); | ||
|
||
resetTimingBuffer(); | ||
|
||
elm.showTop = true; | ||
await Promise.resolve(); | ||
|
||
expect(window.timingBuffer).toEqual([ | ||
'3:connectedCallback', | ||
'4:connectedCallback', | ||
'5:connectedCallback', | ||
'2:disconnectedCallback', | ||
'1:disconnectedCallback', | ||
'0:disconnectedCallback', | ||
]); | ||
|
||
resetTimingBuffer(); | ||
|
||
elm.showTop = false; | ||
await Promise.resolve(); | ||
|
||
expect(window.timingBuffer).toEqual([ | ||
'6:connectedCallback', | ||
'7:connectedCallback', | ||
'8:connectedCallback', | ||
'5:disconnectedCallback', | ||
'4:disconnectedCallback', | ||
'3:disconnectedCallback', | ||
]); | ||
}); | ||
|
||
it('invokes lifecycle methods in correct order - dynamic', async () => { | ||
const elm = createElement('x-dynamic-slot-forwarding', { is: DynamicSlotForwarding }); | ||
document.body.appendChild(elm); | ||
await Promise.resolve(); | ||
|
||
// Initial connection | ||
expect(window.timingBuffer).toEqual([ | ||
'0:connectedCallback', | ||
'1:connectedCallback', | ||
'2:connectedCallback', | ||
]); | ||
|
||
resetTimingBuffer(); | ||
|
||
// Trigger vdom diffing | ||
elm.showTop = true; | ||
await Promise.resolve(); | ||
|
||
expect(window.timingBuffer).toEqual([ | ||
'3:connectedCallback', | ||
'4:connectedCallback', | ||
'5:connectedCallback', | ||
'2:disconnectedCallback', | ||
'1:disconnectedCallback', | ||
'0:disconnectedCallback', | ||
]); | ||
|
||
resetTimingBuffer(); | ||
|
||
// Trigger vdom diffing from top level | ||
elm.topTop = 'bottom'; | ||
elm.topBottom = 'top'; | ||
await Promise.resolve(); | ||
|
||
expect(window.timingBuffer).toEqual([ | ||
'6:connectedCallback', | ||
'5:disconnectedCallback', | ||
'7:connectedCallback', | ||
'3:disconnectedCallback', | ||
]); | ||
|
||
resetTimingBuffer(); | ||
|
||
const { topSlot } = extractDataIds(elm); | ||
// Trigger vdom diffing from forwarded slot level | ||
topSlot.top = 'top'; | ||
topSlot.bottom = 'bottom'; | ||
await Promise.resolve(); | ||
|
||
expect(window.timingBuffer).toEqual([ | ||
'8:connectedCallback', | ||
'6:disconnectedCallback', | ||
'9:connectedCallback', | ||
'7:disconnectedCallback', | ||
]); | ||
|
||
resetTimingBuffer(); | ||
|
||
// vdom diffing after changing vnodes | ||
elm.showTop = false; | ||
await Promise.resolve(); | ||
|
||
expect(window.timingBuffer).toEqual([ | ||
'10:connectedCallback', | ||
'11:connectedCallback', | ||
'12:connectedCallback', | ||
'4:disconnectedCallback', | ||
'9:disconnectedCallback', | ||
'8:disconnectedCallback', | ||
]); | ||
}); | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
let id = 0; | ||
|
||
export const getId = () => id++; | ||
export const resetId = () => (id = 0); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
<template lwc:render-mode="light"> | ||
<x-slottable> | ||
<slot name="top" slot={top}></slot> | ||
<slot></slot> | ||
<slot name="bottom" slot={bottom}></slot> | ||
</x-slottable> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import { LightningElement, api } from 'lwc'; | ||
|
||
export default class extends LightningElement { | ||
static renderMode = 'light'; | ||
|
||
@api top = 'bottom'; | ||
@api bottom = 'top'; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
<template lwc:render-mode="light"> | ||
<template lwc:if={showTop}> | ||
<x-dynamic-forwarded-slottable data-id="topSlot"> | ||
<x-slotee slot={topTop}></x-slotee> | ||
<x-slotee></x-slotee> | ||
<x-slotee slot={topBottom}></x-slotee> | ||
</x-dynamic-forwarded-slottable> | ||
</template> | ||
<template lwc:else> | ||
<x-dynamic-forwarded-slottable data-id="bottomSlot"> | ||
<x-slotee slot={bottomTop}></x-slotee> | ||
<x-slotee></x-slotee> | ||
<x-slotee slot={bottomBottom}></x-slotee> | ||
</x-dynamic-forwarded-slottable> | ||
</template> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import { LightningElement, api } from 'lwc'; | ||
|
||
export default class extends LightningElement { | ||
static renderMode = 'light'; | ||
|
||
@api showTop = false; | ||
@api topTop = 'top'; | ||
@api topBottom = 'bottom'; | ||
@api bottomTop = 'top'; | ||
@api bottomBottom = 'bottom'; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
<template lwc:render-mode="light"> | ||
<x-slottable> | ||
<slot name="top" slot="bottom"></slot> | ||
<slot></slot> | ||
<slot name="bottom" slot="top"></slot> | ||
</x-slottable> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import { LightningElement, api } from 'lwc'; | ||
|
||
export default class extends LightningElement { | ||
static renderMode = 'light'; | ||
|
||
@api top = 'top'; | ||
@api bottom = 'bottom'; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
<template lwc:render-mode="light"> | ||
<template lwc:if={showTop}> | ||
<x-forwarded-slottable> | ||
<x-slotee slot="top"></x-slotee> | ||
<x-slotee></x-slotee> | ||
<x-slotee slot="bottom"></x-slotee> | ||
</x-forwarded-slottable> | ||
</template> | ||
<template lwc:else> | ||
<x-forwarded-slottable> | ||
<x-slotee slot="top"></x-slotee> | ||
<x-slotee></x-slotee> | ||
<x-slotee slot="bottom"></x-slotee> | ||
</x-forwarded-slottable> | ||
</template> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import { LightningElement, api } from 'lwc'; | ||
|
||
export default class extends LightningElement { | ||
static renderMode = 'light'; | ||
|
||
@api showTop = false; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
<template lwc:render-mode="light"> | ||
slottee - {uuid} | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import { LightningElement } from 'lwc'; | ||
import { getId } from '../../util.js'; | ||
|
||
export default class extends LightningElement { | ||
static renderMode = 'light'; | ||
|
||
uuid = getId(); | ||
|
||
connectedCallback() { | ||
window.timingBuffer.push(`${this.uuid}:connectedCallback`); | ||
} | ||
|
||
disconnectedCallback() { | ||
if (window.timingBuffer) { | ||
window.timingBuffer.push(`${this.uuid}:disconnectedCallback`); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
<template lwc:render-mode="light"> | ||
<slot name="top"></slot> | ||
<slot></slot> | ||
<slot name="bottom"></slot> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import { LightningElement } from 'lwc'; | ||
|
||
export default class extends LightningElement { | ||
static renderMode = 'light'; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<template lwc:render-mode="light"> | ||
<template lwc:if={show}> | ||
<x-slottable> | ||
<x-slotee slot="top"></x-slotee> | ||
<x-slotee></x-slotee> | ||
<x-slotee slot="bottom"></x-slotee> | ||
</x-slottable> | ||
</template> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import { LightningElement, api } from 'lwc'; | ||
|
||
export default class extends LightningElement { | ||
static renderMode = 'light'; | ||
|
||
@api show = false; | ||
} |
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
isundefined
, 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:
When the vnode reaches this point in the code, the
div
'svnode.slotAssignment
will beforwardedSlot
, we need to update it to reflectnamedSlot
so that it is slotted into the correct position inx-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.