fix: consistent disconnectedCallback ordering in rendering VFragments #3369

jye-sf · 2023-02-24T22:52:20Z

Details

Fixes an issue with VFragments and inconsistent disconnectedCallback ordering.
Note that because of #2713, there will inherently be some inconsistency in the disconnectedCallback ordering for this use case.

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.

GUS work item

jye-sf · 2023-02-24T23:09:04Z

packages/@lwc/integration-karma/test/rendering/callback-invocation-order/index.spec.js

+        .then(() => {
+            // disconnect x-shadow-parent +
+            // connect x-shadow-container with 2 parents, 'a' and 'b'
+            expect(window.timingBuffer).toEqual([


I checked the ordering here in both synthetic and native shadow against if:true and lwc:if:

if:true - synthetic - 654321 lwc:if - synthetic - 654321 if:true - native - 123456 lwc:if - native - 654321

In synthetic, both if:true and lwc:if are managed by runChildNodesDisconnectedCallback, which uses reverse ordering.
In native, if:true is managed by resetComponentRoot, which uses forward ordering.

lwc:if in native cannot match both the old if:true behavior (forward) and the runChildNodesDisconnectedCallback behavior (reverse). From #2713 it sounds like forward is supposed to be the "correct" behavior, and that's the behavior when conditionals aren't used at all.

IMO since if:true already has different behaviors between synthetic and native, doing the same for lwc:if shouldn't be introducing any confusion that didn't exist before. Keep it consistent (reverse) for synthetic, and correct (forward) for native:

if:true - synthetic - 654321 lwc:if - synthetic - 654321 if:true - native - 123456 lwc:if - native - 123456

Do you see any problems with this approach?

Note that it would not be too difficult to update lwc:if to use forward ordering; it already goes through resetComponentRoot which already does so.

Conclusion from Dev Sync:

if:true - synthetic - 654321 lwc:if - synthetic - 654321 if:true - native - 123456 lwc:if - native - 123456

This will help match native, browser behavior

lwc:if - synthetic - 654321

This is a bit sad, but I would not go out of our way to fix it, since it's synthetic.

jye-sf · 2023-02-24T23:17:39Z

packages/@lwc/integration-karma/test/rendering/callback-invocation-order/index.spec.js

+        })
+        .then(() => {
+            expect(window.timingBuffer).toEqual([
+                'leaf:T2-6:disconnectedCallback',


This step, hiding the content, is interesting because it doesn't follow the same code path as when the template is swapped.

Again, I checked the ordering in both native and synthetic:

if:true - synthetic - 654321 lwc:if - synthetic - 123456 if:true - native - 654321 lwc:if - native - 123456

Here, if:true is always managed by updateStaticChildren, which goes in reverse order.
lwc:if is always managed by unmountVNodes, which goes in forward order.

I believe it's too late and risky to try to change the behavior for if:true, so we have to decide whether we want lwc:if to have the correct forward behavior in both synthetic and native, or figure out how to change it in synthetic to be consistent.

Note that changing the ordering for lwc:if here would be difficult since it goes through unmountVNodes.

Conclusion from dev sync: leave as-is, as we don't guarantee any expectations around ordering of disconnectedCallback when it comes to sibling nodes.

packages/@lwc/integration-karma/test/rendering/callback-invocation-order/index.spec.js

jye-sf · 2023-02-24T23:30:50Z

This PR and its comments are related to the issue brought up in #3363 (comment). Note that I have not included a fix yet because the desired behavior hasn't been confirmed.

@nolanlawson @ravijayaramappa please take a look when you get the chance.

jye-sf · 2023-02-27T18:55:47Z

packages/@lwc/integration-karma/test/rendering/callback-invocation-order/index.spec.js

+            // 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


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

#3369 (comment) for details

jye-sf · 2023-02-27T18:57:01Z

packages/@lwc/integration-karma/test/rendering/callback-invocation-order/index.spec.js

+        })
+        .then(() => {
+            expect(window.timingBuffer).toEqual([
+                'leaf:T2-1:disconnectedCallback',


Updated per #3369 (comment).

jye-sf · 2023-02-27T18:58:12Z

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

+        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);


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

nolanlawson · 2023-02-27T19:00:59Z

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

+// 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.


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.

nolanlawson · 2023-02-27T19:01:22Z

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

-            }
-        }
-    }
+    recursivelyRemoveChildren(vm.children, vm);


Love seeing so much red!

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

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.

nolanlawson

I'm cool with this as long we are not changing the behavior of if:true in any context. lwc:if is new enough that we can probably safely break it with impunity.

Long-term, any other discrepancies should be fixed by #3352 plus migrating to native shadow.

ravijayaramappa

LGTM

…#3369) * fix: add integration tests * fix: update and fix ordering

* fix: @W-12593632 clean up VFragments when resetting component root (#3363) * fix: recursively remove fragment children when resetting component root * fix: consistent disconnectedCallback ordering in rendering VFragments (#3369)

jye-sf added 3 commits February 24, 2023 13:00

fix: add integration tests

17aa113

fix: simplify tests and verify against good behavior

e6f1e8e

fix: enhance tests

94798e1

jye-sf commented Feb 24, 2023

View reviewed changes

jye-sf marked this pull request as ready for review February 24, 2023 23:27

jye-sf mentioned this pull request Feb 24, 2023

fix: @W-12593632 clean up VFragments when resetting component root #3363

Merged

fix: update and fix ordering

57e5f08

jye-sf commented Feb 27, 2023

View reviewed changes

nolanlawson reviewed Feb 27, 2023

View reviewed changes

nolanlawson approved these changes Feb 27, 2023

View reviewed changes

jye-sf merged commit 2bd2bb4 into master Feb 27, 2023

jye-sf deleted the jye/fix-traversal-order branch February 27, 2023 19:54

ravijayaramappa reviewed Feb 27, 2023

View reviewed changes

jye-sf added a commit that referenced this pull request Feb 28, 2023

fix: consistent disconnectedCallback ordering in rendering VFragments (…

a2c4bac

…#3369) * fix: add integration tests * fix: update and fix ordering

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: consistent disconnectedCallback ordering in rendering VFragments #3369

fix: consistent disconnectedCallback ordering in rendering VFragments #3369

jye-sf commented Feb 24, 2023 •

edited

Loading

jye-sf Feb 24, 2023

jye-sf Feb 24, 2023

jye-sf Feb 27, 2023 •

edited

Loading

nolanlawson Feb 27, 2023

jye-sf Feb 24, 2023

jye-sf Feb 24, 2023

jye-sf Feb 27, 2023

jye-sf commented Feb 24, 2023

jye-sf Feb 27, 2023

jye-sf Feb 27, 2023

jye-sf Feb 27, 2023

jye-sf Feb 27, 2023

nolanlawson Feb 27, 2023

nolanlawson Feb 27, 2023

nolanlawson Feb 27, 2023

jye-sf Feb 27, 2023

nolanlawson left a comment

ravijayaramappa left a comment

fix: consistent disconnectedCallback ordering in rendering VFragments #3369

fix: consistent disconnectedCallback ordering in rendering VFragments #3369

Conversation

jye-sf commented Feb 24, 2023 • edited Loading

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jye-sf Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jye-sf commented Feb 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson left a comment

Choose a reason for hiding this comment

ravijayaramappa left a comment

Choose a reason for hiding this comment

jye-sf commented Feb 24, 2023 •

edited

Loading

jye-sf Feb 27, 2023 •

edited

Loading