refactor(engine-core): Simplify unmount process #2709

pmdartus · 2022-02-24T09:12:28Z

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 (#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.

nolanlawson · 2022-02-24T17:35:59Z

packages/@lwc/engine-core/src/framework/vm.ts

    vm.children = EmptyArray;
-
-    runChildNodesDisconnectedCallback(vm);
-    vm.velements = EmptyArray;


Love seeing so much red! ❤️

packages/@lwc/engine-core/src/framework/rendering.ts

packages/@lwc/engine-core/src/framework/vm.ts

packages/integration-karma/test/rendering/disconnect-order/index.spec.js

packages/@lwc/engine-core/src/framework/vm.ts

pmdartus · 2022-03-30T16:46:28Z

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.

nolanlawson · 2022-03-30T16:51:18Z

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 🙂 ).

caridy · 2022-03-30T19:06:08Z

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

ok, I think I got it now.

jmsjtu · 2022-04-08T18:24:31Z

packages/@lwc/engine-core/src/framework/vm.ts

-        runChildNodesDisconnectedCallback(vm);
-        runLightChildNodesDisconnectedCallback(vm);
+
+        unmountChildren(vm.children, vm.renderRoot);


It looks like unmountChildren calls on removeVM which has an assertion check and can potentially throw an error.

I'm not super familiar with this area but it looks like this is a new behavior that might be worth mentioning as a release note or a warning so we don't catch anyone by surprise (unless this assertion is more of an internal check).

caridy · 2022-04-19T03:40:43Z

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

caridy · 2022-04-19T03:47:00Z

@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 · 2022-04-28T03:17:50Z

@jodarove ping

nolanlawson · 2023-03-31T17:38:10Z

@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?

pmdartus · 2023-04-03T11:46:15Z

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.

nolanlawson · 2023-04-03T15:42:34Z

@pmdartus OK, that makes sense. I've opened an issue to track this once we have API versioning: #3438

nolanlawson reviewed Feb 24, 2022

View reviewed changes

pmdartus force-pushed the pmdartus/simplfy-unmount branch from c251502 to c367c5e Compare March 15, 2022 16:10

ravijayaramappa reviewed Mar 15, 2022

View reviewed changes

packages/@lwc/engine-core/src/framework/rendering.ts Outdated Show resolved Hide resolved

packages/@lwc/engine-core/src/framework/vm.ts Show resolved Hide resolved

packages/@lwc/engine-core/src/framework/vm.ts Show resolved Hide resolved

pmdartus force-pushed the pmdartus/simplfy-unmount branch from c367c5e to 7961d7c Compare March 16, 2022 09:24

jodarove suggested changes Mar 23, 2022

View reviewed changes

packages/integration-karma/test/rendering/disconnect-order/index.spec.js Outdated Show resolved Hide resolved

packages/@lwc/engine-core/src/framework/vm.ts Show resolved Hide resolved

pmdartus mentioned this pull request Mar 28, 2022

test: Add connectedCallback invocation order test #2764

Merged

pmdartus force-pushed the pmdartus/simplfy-unmount branch from 0861b56 to 4627b82 Compare March 28, 2022 08:00

pmdartus marked this pull request as ready for review March 31, 2022 09:53

pmdartus added 6 commits March 31, 2022 11:54

refactor: simplify unmount

ac32221

chore: simplify component disconnectedCallback invocation

eacb7ce

refactor: remove velements

ea670d3

chore: address PR feedback

3abe853

chore: update comments

a93359f

test: update tests after rebase

5eaa8b9

pmdartus force-pushed the pmdartus/simplfy-unmount branch from 4627b82 to 5eaa8b9 Compare March 31, 2022 09:54

caridy approved these changes Mar 31, 2022

View reviewed changes

jmsjtu reviewed Apr 8, 2022

View reviewed changes

jodarove self-requested a review April 21, 2022 17:22

nolanlawson mentioned this pull request Aug 4, 2022

Add option to use native connectedCallback/disconnectedCallback lifecycle hooks #2986

Closed

nolanlawson mentioned this pull request Dec 9, 2022

Use native connectedCallback/disconnectedCallback lifecycle hooks #3198

Open

pmdartus closed this Apr 3, 2023

nolanlawson mentioned this pull request Apr 3, 2023

[Observable change] Simplify unmount process #3438

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(engine-core): Simplify unmount process #2709

refactor(engine-core): Simplify unmount process #2709

pmdartus commented Feb 24, 2022

nolanlawson Feb 24, 2022

pmdartus commented Mar 30, 2022

nolanlawson commented Mar 30, 2022 •

edited

Loading

caridy commented Mar 30, 2022

caridy left a comment

jmsjtu Apr 8, 2022

caridy commented Apr 19, 2022

caridy commented Apr 19, 2022

caridy commented Apr 28, 2022

nolanlawson commented Mar 31, 2023

pmdartus commented Apr 3, 2023

nolanlawson commented Apr 3, 2023

refactor(engine-core): Simplify unmount process #2709

refactor(engine-core): Simplify unmount process #2709

Conversation

pmdartus commented Feb 24, 2022

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

nolanlawson Feb 24, 2022

Choose a reason for hiding this comment

pmdartus commented Mar 30, 2022

Current behavior

Proposed behavior

nolanlawson commented Mar 30, 2022 • edited Loading

caridy commented Mar 30, 2022

caridy left a comment

Choose a reason for hiding this comment

jmsjtu Apr 8, 2022

Choose a reason for hiding this comment

caridy commented Apr 19, 2022

caridy commented Apr 19, 2022

caridy commented Apr 28, 2022

nolanlawson commented Mar 31, 2023

pmdartus commented Apr 3, 2023

nolanlawson commented Apr 3, 2023

nolanlawson commented Mar 30, 2022 •

edited

Loading