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

jye-sf · 2023-02-23T20:30:49Z

Details

VFragments are not cleaned up properly when resetting component root because their .elm points to the end delimiter text node, not the root element.

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-23T20:36:03Z

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

-        if (!isNull(child) && !isUndefined(child.elm)) {
+        // 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) && isVFragment(child)) {


VFragment is the only VNode that represents a set of VNodes and has no root. I couldn't think of a way to deal with the them in a generic way here, so we just check whether this is a Fragment and react accordingly.

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

jye-sf · 2023-02-23T20:38:16Z

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

+        // 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) && isVFragment(child)) {
+            removeFragmentChildren(child, vm);


I debated just converting this entire function to traverse using a stack so we wouldn't need a separate function, but this felt like a cleaner way to isolate the impact of VFragments on non-vfragment use cases.

jye-sf · 2023-02-23T20:43:55Z

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

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


Because VFragments can be nested inside other VFragments, we can't just remove the immediate children of the first fragment we find. We need to traverse the entire tree and grab all children that are immediate children of the root node.

<template> <div>Immediate child of root</div> VFragment( <div>Nested 1: also immediate child of root</div> VFragment( <div>Nested 2: also immediate child of root</div> ) ) </template>

<x-custom-component> <div>Immediate child of root</div> <div>Nested 1: also immediate child of root</div> <div>Nested 2: also immediate child of root</div> </x-custom-component>

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

nolanlawson · 2023-02-23T22:35:14Z

packages/@lwc/integration-karma/test/rendering/if-inside-custom-render/index.spec.js

+import CustomRender from 'x/customRender';
+
+describe('using custom renderer with lwc:if', () => {
+    it('should just work test debug', async function () {


Did you mean to name it this way? :)

I thought I changed this....that's embarrassing 😓

We've all been there. I like yolo haha myself

nolanlawson · 2023-02-23T22:36:00Z

scripts/bundlesize/bundlesize.config.json

@@ -2,7 +2,7 @@
    "files": [
        {
            "path": "packages/lwc/dist/engine-dom/umd/es2017/engine-dom.min.js",
-            "maxSize": "20KB"
+            "maxSize": "20.02KB"


Feel free to bump to 20.5 or 21; no need to be right above the limit :)

nolanlawson

LGTM, I am thinking the risk is quite low for the observable change. The previous behavior seems truly buggy, and lwc:if is relatively new

jye-sf · 2023-02-23T22:37:07Z

Thread model / Risk assessment:

To get this behavior, a customer would need to:

use lwc:if in a template
lwc:if should be an immediate child of the root node (top level element on the component)
swap out an initial template with a new template containing the lwc:if (are there other use cases for this besides custom render function?)

The end user behavior:

the old template is not unrendered. The new template is rendered and appended to the old template.

Other notes:

this is a new feature in 242, so it's unlikely for customers to have existing implementations.
If a user is migrating from if:true to lwc:if, and they match this use case, their implementation will break/change. Users are unlikely to have migrated to relying on this 'new' behavior.
if a new user is attempting to swap out one template for a new template, they're unlikely to rely on behavior that appends the new template to their old template.

I think this has low risk to break customers, especially since we're limiting the change to VFragment nodes which are also new to 242.

nolanlawson · 2023-02-23T22:44:32Z

swap out an initial template with a new template containing the lwc:if (are there other use cases for this besides custom render function?)

So effectively a multi-template component with an <lwc:if> at the top level of each template? No, this sounds like the only case where this would apply.

Also, the behavior is totally busted from the user's POV (they tried to swap out another template, it didn't work), so it seems unlikely anyone would take a dependency on it

jye-sf · 2023-02-23T22:48:09Z

So effectively a multi-template component with an lwc:if at the top level of each template?

Yes that's correct.

…3363) * fix: add initial test case * fix: recursively remove fragment children when resetting component root

ravijayaramappa · 2023-02-24T19:29:08Z

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

+    while (!isUndefined((currentNode = ArrayPop.call(nodeStack)))) {
+        if (!isNull(currentNode)) {
+            if (isVFragment(currentNode)) {
+                ArrayPush.call(nodeStack, ...currentNode.children);


The order of removal here is not following tree order. Not a problem now, because disconnectedCallback is being managed manually by runChildNodesDisconnectedCallback. When the switch is made to native disconnectedCallback for custom elements, this will need to be fixed.

fyi @nolanlawson

Thanks for calling that out. We should change it to match the disconnect order.

Good catch @ravijayaramappa !

@ravijayaramappa Looked into it a bit, you're right in that there's an issue with disconnectedCallback ordering, but not in the way you described; the order of removal here actually does match the order in runChildNodesDisconnectedCallback; both remove/disconnect in reverse sibling order.

However, other issues have come up, and I'm hitting up against the inconsistent disconnect ordering mentioned in #2713. When you have some time, could you take a look at #3369 and give your input on what the "correct" order of removal should be in the various use cases?

* 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 4 commits February 23, 2023 12:28

fix: add initial test case

4a53fa8

fix: update test with nested case

d51c4ff

fix: recursively remove fragment children when resetting component root

f232e76

fix: simplify test

46a2ee5

jye-sf mentioned this pull request Feb 23, 2023

fix: clean up VFragments when resetting component root #3360

Merged

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

fix: update formatting

417d42b

jye-sf commented Feb 23, 2023

View reviewed changes

jye-sf added 2 commits February 23, 2023 12:58

fix: update test for native

d229038

fix: update bundlesize config

642a449

nolanlawson reviewed Feb 23, 2023

View reviewed changes

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

nolanlawson reviewed Feb 23, 2023

View reviewed changes

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

nolanlawson reviewed Feb 23, 2023

View reviewed changes

nolanlawson approved these changes Feb 23, 2023

View reviewed changes

fix: better boolean logic, rename test, bundlesize bump

612d754

jye-sf merged commit 551f634 into master Feb 24, 2023

jye-sf deleted the jye/fix-render-reset-master branch February 24, 2023 16:52

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

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

9a2e126

…3363) * fix: add initial test case * fix: recursively remove fragment children when resetting component root

ravijayaramappa reviewed Feb 24, 2023

View reviewed changes

jye-sf mentioned this pull request Feb 24, 2023

fix: consistent disconnectedCallback ordering in rendering VFragments #3369

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jye-sf commented Feb 23, 2023

jye-sf Feb 23, 2023

jye-sf Feb 23, 2023

jye-sf Feb 23, 2023

nolanlawson Feb 23, 2023

jye-sf Feb 23, 2023

nolanlawson Feb 23, 2023

nolanlawson Feb 23, 2023

nolanlawson left a comment

jye-sf commented Feb 23, 2023 •

edited

Loading

nolanlawson commented Feb 23, 2023 •

edited

Loading

jye-sf commented Feb 23, 2023

ravijayaramappa Feb 24, 2023

jye-sf Feb 24, 2023

nolanlawson Feb 24, 2023

jye-sf Feb 24, 2023

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

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

Conversation

jye-sf commented Feb 23, 2023

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

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

jye-sf commented Feb 23, 2023 • edited Loading

nolanlawson commented Feb 23, 2023 • edited Loading

jye-sf commented Feb 23, 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

jye-sf commented Feb 23, 2023 •

edited

Loading

nolanlawson commented Feb 23, 2023 •

edited

Loading