Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: consistent disconnectedCallback ordering in rendering VFragments #3369

Merged
merged 4 commits into from
Feb 27, 2023
Merged

fix: consistent disconnectedCallback ordering in rendering VFragments #3369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
17aa113
fix: add integration tests
jye-sf Feb 24, 2023
e6f1e8e
fix: simplify tests and verify against good behavior
jye-sf Feb 24, 2023
94798e1
fix: enhance tests
jye-sf Feb 24, 2023
57e5f08
fix: update and fix ordering
jye-sf Feb 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 15 additions & 42 deletions packages/@lwc/engine-core/src/framework/vm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/
import {
ArrayPop,
ArrayPush,
ArraySlice,
ArrayUnshift,
Expand Down Expand Up @@ -44,15 +43,7 @@ import { patchChildren } from './rendering';
import { ReactiveObserver } from './mutation-tracker';
import { connectWireAdapters, disconnectWireAdapters, installWireAdapters } from './wiring';
import { removeActiveVM } from './hot-swaps';
import {
VNodes,
VCustomElement,
VNode,
VNodeType,
VBaseElement,
VFragment,
isVFragment,
} from './vnodes';
import { VNodes, VCustomElement, VNode, VNodeType, VBaseElement, isVFragment } from './vnodes';
import { StylesheetFactory, TemplateStylesheetFactories } from './stylesheet';

type ShadowRootMode = 'open' | 'closed';
Expand Down Expand Up @@ -739,50 +730,32 @@ function recursivelyDisconnectChildren(vnodes: VNodes) {
// into snabbdom. Especially useful when the reset is a consequence of an error, in which case the
// children VNodes might not be representing the current state of the DOM.
export function resetComponentRoot(vm: VM) {
const {
children,
renderRoot,
renderer: { remove },
} = vm;

for (let i = 0, len = children.length; i < len; i++) {
const child = children[i];

// VFragments are special; their .elm property does not point to the root element since they have no root,
// so we have to clean them up differently.
if (!isNull(child)) {
if (isVFragment(child)) {
removeFragmentChildren(child, vm);
} else if (!isUndefined(child.elm)) {
remove(child.elm, renderRoot);
}
}
}
recursivelyRemoveChildren(vm.children, vm);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love seeing so much red!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am surprised that this does not introduce an observable change to if:true. But if it doesn't, then let's go ahead. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably because the logic is overall still the same for if:true. We went from:

for (let i = 0, len = children.length; i < len; i++) {
    const child = children[i];

    if (!isNull(child) && !isUndefined(child.elm)) {
        remove(child.elm, renderRoot);
    }
}

to

for (let i = 0, len = vnodes.length; i < len; i += 1) {
    const vnode = vnodes[i];

    if (!isNull(vnode)) {
        if (isVFragment(vnode)) {
            recursivelyRemoveChildren(vnode.children, vm);
        } else if (!isUndefined(vnode.elm)) {
            remove(vnode.elm, renderRoot);
        }
    }
}

No existing use cases should go down the new recursive path we've added.

vm.children = EmptyArray;

runChildNodesDisconnectedCallback(vm);
vm.velements = EmptyArray;
}

// Helper function to traverse a tree of VFragment nodes and remove all root children.
// This is moved into a separate function to minimize the perf/mem impact of the stack traversal
// on non-vfragment use cases
function removeFragmentChildren(vnode: VFragment, vm: VM) {
// Helper function to remove all children of the root node.
// If the set of children includes VFragment nodes, we need to remove the children of those nodes too.
// Since VFragments can contain other VFragments, we need to traverse the entire of tree of VFragments.
// If the set contains no VFragment nodes, no traversal is needed.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a comment here noting that we are going in forward-tree order to match native disconnectedCallback ordering when removing a top-level node.

function recursivelyRemoveChildren(vnodes: VNodes, vm: VM) {
const {
renderRoot,
renderer: { remove },
} = vm;

const nodeStack: VNode[] = [];
ArrayPush.call(nodeStack, ...vnode.children);
for (let i = 0, len = vnodes.length; i < len; i += 1) {
const vnode = vnodes[i];

let currentNode: VNode | null | undefined;
while (!isUndefined((currentNode = ArrayPop.call(nodeStack)))) {
if (!isNull(currentNode)) {
if (isVFragment(currentNode)) {
ArrayPush.call(nodeStack, ...currentNode.children);
} else if (!isUndefined(currentNode.elm)) {
remove(currentNode.elm, renderRoot);
if (!isNull(vnode)) {
// VFragments are special; their .elm property does not point to the root element since they have no single root.
if (isVFragment(vnode)) {
recursivelyRemoveChildren(vnode.children, vm);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching back to recursion for forward traversal. Matches the traversal behavior of recursivelyDisconnectChildren in line 708.

} else if (!isUndefined(vnode.elm)) {
remove(vnode.elm, renderRoot);
}
}
}
Expand Down
69 changes: 69 additions & 0 deletions packages/@lwc/integration-karma/test/rendering/callback-invocation-order/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import ShadowLightParent from 'x/shadowLightParent';
import LightParent from 'x/lightParent';
import LightShadowParent from 'x/lightShadowParent';
import ToggleContainer from 'x/toggleContainer';
import MultiTemplateConditionals from 'x/multiTemplateConditionals';

function resetTimingBuffer() {
window.timingBuffer = [];
Expand Down Expand Up @@ -228,3 +229,71 @@ it('should invoke callbacks on the right order (issue #1199 and #1198)', () => {
);
});
});

it('should invoke callbacks on the right order when multiple templates are used with lwc:if', () => {
jye-sf marked this conversation as resolved.
Show resolved Hide resolved
const elm = createElement('x-multi-template-conditionals', { is: MultiTemplateConditionals });
elm.show = true;
document.body.appendChild(elm);

// initial load is x-shadow-parent
expect(window.timingBuffer).toEqual([
'leaf:T1-1:connectedCallback',
'leaf:T1-2:connectedCallback',
'leaf:T1-3:connectedCallback',
'leaf:T1-4:connectedCallback',
'leaf:T1-5:connectedCallback',
'leaf:T1-6:connectedCallback',
]);

resetTimingBuffer();
elm.next();

return Promise.resolve()
.then(() => {
// disconnect x-shadow-parent +
// connect x-shadow-container with 2 parents, 'a' and 'b'
expect(window.timingBuffer).toEqual(
window.lwcRuntimeFlags.ENABLE_NATIVE_CUSTOM_ELEMENT_LIFECYCLE
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated for the desired behavior in native lifecycle vs synthetic lifecycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3369 (comment) for details

? [
'leaf:T1-1:disconnectedCallback',
'leaf:T1-2:disconnectedCallback',
'leaf:T1-3:disconnectedCallback',
'leaf:T1-4:disconnectedCallback',
'leaf:T1-5:disconnectedCallback',
'leaf:T1-6:disconnectedCallback',
'leaf:T2-1:connectedCallback',
'leaf:T2-2:connectedCallback',
'leaf:T2-3:connectedCallback',
'leaf:T2-4:connectedCallback',
'leaf:T2-5:connectedCallback',
'leaf:T2-6:connectedCallback',
]
: [
'leaf:T1-6:disconnectedCallback',
'leaf:T1-5:disconnectedCallback',
'leaf:T1-4:disconnectedCallback',
'leaf:T1-3:disconnectedCallback',
'leaf:T1-2:disconnectedCallback',
'leaf:T1-1:disconnectedCallback',
'leaf:T2-1:connectedCallback',
'leaf:T2-2:connectedCallback',
'leaf:T2-3:connectedCallback',
'leaf:T2-4:connectedCallback',
'leaf:T2-5:connectedCallback',
'leaf:T2-6:connectedCallback',
]
);
resetTimingBuffer();
elm.show = false;
})
.then(() => {
expect(window.timingBuffer).toEqual([
'leaf:T2-1:disconnectedCallback',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated per #3369 (comment).

'leaf:T2-2:disconnectedCallback',
'leaf:T2-3:disconnectedCallback',
'leaf:T2-4:disconnectedCallback',
'leaf:T2-5:disconnectedCallback',
'leaf:T2-6:disconnectedCallback',
]);
});
});
25 changes: 25 additions & 0 deletions ...dering/callback-invocation-order/x/multiTemplateConditionals/multiTemplateConditionals.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { LightningElement, api } from 'lwc';
import template from './template.html';
import template2 from './template2.html';

export default class MultiTemplateConditionals extends LightningElement {
templateIndex = 0;
templateMapping = {
0: template,
1: template2,
};

@api
show = false;

render() {
return this.templateMapping[this.templateIndex];
}

@api
next() {
if (this.templateIndex < 1) {
this.templateIndex++;
}
}
}
14 changes: 14 additions & 0 deletions ...-karma/test/rendering/callback-invocation-order/x/multiTemplateConditionals/template.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<template>
<template lwc:if={show}>
<x-leaf name="T1-1"></x-leaf>
<template lwc:if={show}>
<x-leaf name="T1-2"></x-leaf>
<template lwc:if={show}>
<x-leaf name="T1-3"></x-leaf>
<x-leaf name="T1-4"></x-leaf>
</template>
<x-leaf name="T1-5"></x-leaf>
</template>
<x-leaf name="T1-6"></x-leaf>
</template>
</template>
14 changes: 14 additions & 0 deletions ...karma/test/rendering/callback-invocation-order/x/multiTemplateConditionals/template2.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<template>
<template lwc:if={show}>
<x-leaf name="T2-1"></x-leaf>
<template lwc:if={show}>
<x-leaf name="T2-2"></x-leaf>
<template lwc:if={show}>
<x-leaf name="T2-3"></x-leaf>
<x-leaf name="T2-4"></x-leaf>
</template>
<x-leaf name="T2-5"></x-leaf>
</template>
<x-leaf name="T2-6"></x-leaf>
</template>
</template>