lwc icon indicating copy to clipboard operation
lwc copied to clipboard

refactor(engine-core): Simplify unmount process

Open pmdartus opened this issue 3 years ago • 6 comments

Details

This PR streamlines the component unmounting process. Currently, the component unmounting logic resides in the vm.ts and uses a different logic than the diffing algo. This PR corrects this and leverages the existing diffing algo to unmount components, resulting in smaller bundle size.

Another noteworthy change of this PR is the removal of the velements property on the VM. The property tracks all the components rendered in the shadow tree, to speed up the unmounting process. This would enable hoisting custom element VNode outside the render function (https://github.com/salesforce/lwc/issues/2688). My current assumption is VDOM traversal is fast enough and the performance impact would be minimal. This has to be validated with new performance benchmarks.

Does this pull request introduce a breaking change?

  • ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • ⚠️ Yes, it does include an observable change.

Currently, components in the shadow tree are unmounted in revered insertion order, while allocated children (slotted synthetic shadow DOM elements and slotted light DOM elements) are unmounted in insertion order.

With this PR, all the components are unmounted in insertion order, which changes the invocation order for disconnectedCallback. Additional test coverage is needed to validate this.

pmdartus avatar Feb 24 '22 09:02 pmdartus

This PR raised some questions about lifecycle callback invocation timings, the difference between native and synthetic shadow DOM, the difference between native custom elements and LWC. Let me try to summarize this.


Let's take a concrete example to illustrate this. Consider a component tree composed of 3 components:

  • <x-leaf>: renders an empty shadow tree
  • <x-container>: renders a shadow tree containing a <slot> element surrounded by 2 <x-leaf> components
  • <-parent>: renders a shadow tree containing an <x-container> components surrounded by 2 <x-leaf> components. The <x-container> component has another <x-leaf> component as a light DOM child.

Here is the composed component tree.

<x-parent id="parent">
    #shadow-root
    |   <x-leaf id="before-container"></x-leaf>
    |   <x-container id="container">
    |       #shadow-root
    |       |   <x-leaf id="before-slot"></x-leaf>
    |       |   <slot></slot>
    |       |   <x-leaf id="after-slot"></x-leaf> 
    |       <x-leaf id="slotted"></x-leaf>
    |   </x-container>
    |   <x-leaf id="after-container"></x-leaf>
</x-parent>

I reimplemented the same component tree with native custom elements (jsbin) and with LWC (source). All the components implements the connectedCallback and disconnectedCallback lifecycle hooks and logs to console when invoked.

Current behavior

Native custom element LWC (native shadow DOM) LWC (synthetic shadow DOM)
Mount x-parent (id: parent)
x-leaf (id: before-container)
x-container (id: container)
x-leaf (id: before-slot)
x-leaf (id: after-slot)
x-leaf (id: slotted)
x-leaf (id: after-container)
x-parent (id: parent)
x-leaf (id: before-container)
x-container (id: container)
x-leaf (id: slotted)
x-leaf (id: before-slot)
x-leaf (id: after-slot)
x-leaf (id: after-container)
x-parent (id: parent)
x-leaf (id: before-container)
x-container (id: container)
x-leaf (id: before-slot)
x-leaf (id: slotted)
x-leaf (id: after-slot)
x-leaf (id: after-container)
Unmount x-parent (id: parent)
x-leaf (id: before-container)
x-container (id: container)
x-leaf (id: before-slot)
x-leaf (id: after-slot)
x-leaf (id: slotted)
x-leaf (id: after-container)
x-parent (id: parent)
x-leaf (id: after-container)
x-container (id: container)
x-leaf (id: after-slot)
x-leaf (id: before-slot)
x-leaf (id: slotted)
x-leaf (id: before-container)
x-parent (id: parent)
x-leaf (id: after-container)
x-container (id: container)
x-leaf (id: after-slot)
x-leaf (id: before-slot)
x-leaf (id: slotted)
x-leaf (id: before-container)

Observations:

  • The connectedCallback are invoked in the same order with the exception of the slotted <x-leaf> element:
    • Native custom elements - invoked once the <x-container> is done connecting and rendering its shadow DOM
    • LWC (native shadow DOM) - invoked after the <x-container> is connected but before its shadow tree components are connected.
    • LWC (synthetic shadow DOM) - invoked when the <slot> element is connected in <x-container>.
  • LWC disconnectedCallback invocation timing greatly differs from the native customer one. The shadow tree elements are unmounted in reversed tree order when native custom elements are unmounted in tree order.

Proposed behavior

This PR doesn't only affect disconnectedCallback invocation timing. The connectedCallback invocation timing is preserved.

Native custom element LWC (native shadow DOM) LWC (synthetic shadow DOM)
Unmount x-parent (id: parent)
x-leaf (id: before-container)
x-container (id: container)
x-leaf (id: before-slot)
x-leaf (id: after-slot)
x-leaf (id: slotted)
x-leaf (id: after-container)
x-parent (id: parent)
x-leaf (id: before-container)
x-container (id: container)
x-leaf (id: before-slot)
x-leaf (id: after-slot)
x-leaf (id: slotted)
x-leaf (id: after-container)
x-parent (id: parent)
x-leaf (id: before-container)
x-container (id: container)
x-leaf (id: before-slot)
x-leaf (id: slotted)
x-leaf (id: after-slot)
x-leaf (id: after-container)

Proposed changes:

  • Components are now unmounted in tree order. This matches closer with the native custom element behavior.
  • Furthermore, this PR changes the way components are disconnected to match the order in which they are connected. With synthetic shadow DOM, slotted elements are disconnected lazily. With native shadow DOM, slotted elements are disconnected eagrly.

This PR doesn't attempt to solve all the invocation timing issues we have with native custom elements. It only attempts to course-correct the obvious discrepancies with have with disconnectedCallback and brings some consistency with the order in which connectedCallback is invoked.

pmdartus avatar Mar 30 '22 16:03 pmdartus

Thanks for the analysis @pmdartus. It's important to know where we differ from the native behavior and to try to reduce it as much as possible.

This PR doesn't attempt to solve all the invocation timing issues we have with native custom elements.

Yep, based on my testing with #2724 and using native lifecycle callbacks in LWC, the main difference in timing is not in the ordering, but in the DOM / event loop timing. I.e. the ordering between the components may be the same, but under the hood, there are subtle differences (microtasks? rAF? I didn't check TBH). This is something we'll have to analyze when/if we switch to native lifecycle callbacks (all I can tell you is that it breaks plenty of our Karma tests 🙂 ).

nolanlawson avatar Mar 30 '22 16:03 nolanlawson

I don't fully understand this PR, not sure how are you doing it without holding back a reference to those vnodes... but in principle, the idea described above does make sense to me. If the tests are all passing, I'm fine with this.

caridy avatar Mar 30 '22 19:03 caridy

@pmdartus what is missing here? when do we plan to merge this?

caridy avatar Apr 19 '22 03:04 caridy

@pmdartus with this refactor, is there a need for vnode.vm anymore? I think it is important for us to remove that pointer, which is set lazily and used only in a couple of places. the reason why I think this is important is due to custom elements and multi-engine, there is no guarantee that a vnode will ever get to know the vm associated to it, it should not care.

caridy avatar Apr 19 '22 03:04 caridy

@jodarove ping

caridy avatar Apr 28 '22 03:04 caridy

@pmdartus It seems unlikely that we're going to merge this, and the eventual solution will be native custom element lifecycle callbacks. Should we close this PR?

nolanlawson avatar Mar 31 '23 17:03 nolanlawson

Yes, let me close this PR.

I am not sure that the native CE lifecycle will solve this discrepancy. The core issue resides in the order we patch elements in the DOM. Using native lifecycle hooks will not change this as far as I understand.

pmdartus avatar Apr 03 '23 11:04 pmdartus

@pmdartus OK, that makes sense. I've opened an issue to track this once we have API versioning: https://github.com/salesforce/lwc/issues/3438

nolanlawson avatar Apr 03 '23 15:04 nolanlawson