lwc icon indicating copy to clipboard operation
lwc copied to clipboard

Incorrect callback invocation order when swapping component with lwc:dynamic

Open pmdartus opened this issue 4 years ago • 7 comments

Description

When swapping a component dynamically, the new component is created and connected before the old one is disconnected. The old component should be disconnected from the tree before the new one replaces it.

Steps to Reproduce

Playground: Link. Click on the switch dynamic component button and look at the console.

// c/main/main.js
import { LightningElement, track } from 'lwc';
import A from 'c/a';
import B from 'c/b';

export default class HelloApp extends LightningElement {
    @track viewCtor = A;
    switchCmp() {
        this.viewCtor = this.viewCtor === A ? B : A;
    }
 }
<!-- c/main/main.html -->
<template>
    <main>
        <h1>Hello World!</h1>
        <hello-dynamic lwc:dynamic={viewCtor}></hello-dynamic>
        <a onclick={switchCmp}>switch dynamic component</a>
    </main>
</template>
// c/a/a.js
import { LightningElement } from 'lwc';

export default class HelloA extends LightningElement {
    connectedCallback() {
       console.log('Connected component A');
    }
    disconnectedCallback() {
        console.log('Disconnected component A');
    }
}
// c/b/b.js
import { LightningElement } from 'lwc';

export default class HelloB extends LightningElement {
    connectedCallback() {
        console.log('Connected component B');
    }
    disconnectedCallback() {
        console.log('Disconnected component B');
    }
}

Expected Results

Disconnected component A
Connected component B

Actual Results

Connected component B
Disconnected component A

Browsers Affected

All

Version

  • LWC: 1.6.0

pmdartus avatar May 26 '20 14:05 pmdartus

This issue has been linked to a new work item: W-7617413

git2gus[bot] avatar May 26 '20 14:05 git2gus[bot]

@pmdartus i took a quick look to this issue. at the moment that the new component is connected, the older one is still in the DOM. this is because of the diffing algo, that removes old vnodes after it adds/patch the new ones.

these are 2 different components, and imo the order in which the connected/disconnected callback is called should not matter.

will the order matters in this other example: https://playground.lwcjs.org/projects/s18WOrkrx/2/edit ?

jodarove avatar Jun 26 '20 00:06 jodarove

In the example, you swap component by conditionally rendering them.

<template>
    <template if:true={visible}>
        <c-a></c-a>
    </template>
    <template if:false={visible}>
        <c-b></c-b>
    </template>
</template>

The fey difference between this example and the example with lwc:dynamic is that in the first one the template renders 2 different DOM elements and in the latter, the 2 component instances share the same host element. If you look at the invocation order, component B is connected before component A is connected. This means, at a given point in time, the 2 component instances are both connected. This can't be the case because they should share the same host element.

Will the order matters in this other example: https://playground.lwcjs.org/projects/s18WOrkrx/2/edit ?

Yes, the order does also matter in this example as well. This is really unexpected that updateStaticChildren traverses the list of VNodes backward. https://github.com/salesforce/lwc/blob/17f3289c3591a3e125384a056dcf49492e179f24/packages/%40lwc/engine-core/src/3rdparty/snabbdom/snabbdom.ts#L170

The <c-a> lifecycle hook should always be invoked before the <c-b> lifecycle hook due to its position in the DOM. The initial render does invoke the lifecycle hook in the right order but the subsequent ones are not.

pmdartus avatar Jun 26 '20 07:06 pmdartus

from the very beginning, since we choose snabbdom, we were very clear that the order in which things are created/connected/disconnected is NOT deterministic at the sibling level. When did that assumption change? why do you think this is important? yes, it is observable, but not deterministic.

As for lwc:dynamic, that should not be different from a iterable list. It is key based, which will determine the order or reusability of the elements. In fact, the example above will be static, and no elements will be reused, similarly, lwc:dynamic will have the same characteristics, it will never reuse the element.

IMO, this is not an issue.

caridy avatar Jun 26 '20 17:06 caridy

I don't recall any discussion around the fact that the component and patching order is not deterministic. Nor I don't see any reference to this in the codebase or in the documentation. Let say that this is not a bug but rather a feature request.

The question was asked as follows: When using the lwc:dynamic directive to switch components, is there an expected order in which they are connected and disconnected? Based on the current engine implementation, the answer is no. The original example was an LWC router component rendering dynamically a page using lwc:dynamic to swap the entire page content. It was observed that a certain point in time the 2 pages were renderer at the same time in the DOM. This was an unexpected behavior, for them and for me. In the component template, there is a single element with an lwc:dynamic directive and the expected behavior it that there will also be only one component connected at the time. If the LWC component constructor happen to change, I would expect the old component to be disconnected first and the new one to be connected later.

Generally speaking, the mental model I had so far when it comes to the way the engine creates and patch the DOM is that it follows the standard DOM traversal. The same way that the ShadowRoot.innerHTML setter parses and creates the DOM nodes in a specific order. It was really unexpected to me, that the patching order differs from the creation order.

Should we address it? I think we should.

The fact that we are using snabddom, is an implementation detail and should not be considered in the discussion. The patching algorithm is at the center of the framework, and stating that it has a non-deterministic behavior makes me really nervous because it might be a source of undefined behavior for consumers.

pmdartus avatar Jun 29 '20 12:06 pmdartus

lets chat about this... I think you're pursuing something that is impossible to achieve... every developer will have its own mental model about how the update of the dom should be done... you're making mutations to the dom, how does mutation are done is very tricky to make it deterministic... what matter most is when can we consider those mutations done... when it is safe for me to interact with the fragment.

Any determinism that you try to add to this is kinda useless if the browser will eventually introduce their own template system that you can use to apply the changes.

caridy avatar Jul 01 '20 04:07 caridy

Is this related to https://github.com/salesforce/lwc/issues/1476 ?

nolanlawson avatar Aug 04 '22 23:08 nolanlawson

Closing this because lwc:dynamic is being redesigned.

ravijayaramappa avatar Jan 19 '23 17:01 ravijayaramappa