refactor: fix light dom slots for fragments #3824
Conversation
| it('regression test (#3827) - light DOM should maintain callback invocation order for duplicate slots across conditional branches when rerender triggered on conditional slot component', () => { | ||
| // #TODO[3827]: These regression tests can be removed once the bug is resolved. | ||
| const container = createElement('x-light-container-multiple-conditionals', { | ||
| is: LightContainerMultipleConditionals, | ||
| }); | ||
| document.body.appendChild(container); | ||
|
|
||
| let currentLeafName = container.getLeaf().name; | ||
| expect(window.timingBuffer).toEqual([ | ||
| 'lightContainer:connectedCallback', | ||
| 'lightSlot:connectedCallback', | ||
| `leaf:${currentLeafName}:connectedCallback`, | ||
| ]); |
There was a problem hiding this comment.
These tests are a little strange, but I created them to capture the current behavior and ensure they match what happens in 3.x.x.
There was a problem hiding this comment.
Note that this bug only occurs if the conditional rendering occurs on the same component as the <slot> element.
The bug also only occurs when the condition is only triggered through the component containing the <slot>.
If the condition depends on a value being passed from a parent component, the slots will render correctly.
packages/@lwc/integration-karma/test/rendering/callback-invocation-order/index.spec.js
Outdated
Show resolved
Hide resolved
packages/@lwc/integration-karma/test/rendering/callback-invocation-order/index.spec.js
Outdated
Show resolved
Hide resolved
packages/@lwc/integration-karma/test/rendering/callback-invocation-order/index.spec.js
Outdated
Show resolved
Hide resolved
| : [ | ||
| `leaf:${currentLeafName}:connectedCallback`, | ||
| `leaf:${currentLeafName}:disconnectedCallback`, | ||
| ] |
There was a problem hiding this comment.
So the previous leaf never fires a disconnectedCallback? I thought this was the entire bug?
There was a problem hiding this comment.
@nolanlawson yup, the previous leaf won't fire disconnectedCallback in synthetic lifecycle.
This is how it behaves in 3.x.x, this PR just brings the behavior back to what it was in 3.x.x.
I plan to raise another PR to fix the disconnectedCallback behavior entirely as part of #3832
There was a problem hiding this comment.
I plan to raise another PR to fix the
disconnectedCallbackbehavior entirely
Is this worth doing if we are going to do native lifecycle in 5.0?
There was a problem hiding this comment.
Yeah I think either way this will need to be fixed because if you look at the native lifecycle (one above this one) the new component is connected and immediately disconnected.
Which means all the components are disconnected after unmount runs.
There was a problem hiding this comment.
Also, this is a very specific bug, the rerender has to be isolated to just the component containing the <slot> element.
For example:
<template>
<template lwc:if={showMe}>
<slot></slot>
<template lwc:else>
<slot></slot>
</template>If the value of showMe is passed down from a parent component then this bug does not appear, but if the value of showMe is isolated to just the component, it does.
There was a problem hiding this comment.
Yeah I think either way this will need to be fixed because if you look at the native lifecycle (one above this one) the new component is connected and immediately disconnected.
So is the issue that the diffing algo is doing extra unnecessary work?
There was a problem hiding this comment.
Sort of, in this specific bug, the references to what's being slotted is getting mixed up, so when the element is removed from the DOM the native lifecycle is called on the old component and the synthetic lifecycle is called on the new component.
Details
Partially fixes #3823, the synthetic lifecycle will not call
disconnectedCallbackbut does bring it back to parity with the behavior from LWC 3.x.x.A followup PR will fully resolve #3823.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
This will allow
disconnectedCallbackto be invoked for light DOM slots, which was merged today so it shouldn't have a large impact.