[FIX] components: stop rendering stale t-component when delayed

Previously, if the value of a t-component directive changed, but there
was already a scheduled render for the component before that change, the
rendering of the existing component would get delayed (as it should),
but after the parent's rendering was complete, the old component which
will be destroyed during the patch would still get rendered, despite
being stale. This can cause crashes if that component relies on state
that no longer exists.

This was caused by the fact that when rendering, we check the parent
chain for an active render, and also check that the component still
exists in the parent's fiber childrenMap (ie, we know that the upcoming
patch is not about to destroy the component). To do this, we use the
component's parentKey, but in the case of t-component, the parentKey for
both possible components is the same, causing the stale component to
incorrectly believe that it's not about to be destroyed, by finding the
next component in the childrenMap under the same key.

This commit fixes that by prepending the component's name to the
parentKey, meaning that if the value of the t-component changes, so will
the key, and the render will be further delayed until the new state is
patched, at which point the stale component will be destroyed and the
following attempt to render will be cancelled as expected.

The choice to use the component's name is to simplify the
implementation, this means that if using t-component and switching
between components that have the same class name, this protection will
not work. The alternative would be to generate a unique id for the
component class, eg with a WeakMap, but this both complicates the
implementation and adds runtime overhead, for a case that is likely
extremely rare. We are open to refine the implementation should this
problem occur in practice.

src/compiler/code_generator.ts

@@ -1223,6 +1223,13 @@ export class CodeGenerator {
       })`,
     });
 
+    if (ast.isDynamic) {
+      // If the component class changes, this can cause delayed renders to go
+      // through if the key doesn't change. Use the component name for now.
+      // This means that two component classes with the same name isn't supported
+      // in t-component. We can generate a unique id per class later if needed.
+      keyArg = `(${expr}).name + ${keyArg}`;
+    }
     let blockExpr = `${id}(${propString}, ${keyArg}, node, this, ${ast.isDynamic ? expr : null})`;
     if (ast.isDynamic) {
       blockExpr = `toggler(${expr}, ${blockExpr})`;

tests/components/__snapshots__/basics.test.ts.snap

@@ -8,7 +8,7 @@ exports[`basics GrandChild display is controlled by its GrandParent 1`] = `
   
   return function template(ctx, node, key = \\"\\") {
     const Comp1 = ctx['myComp'];
-    return toggler(Comp1, comp1({displayGrandChild: ctx['displayGrandChild']}, key + \`__1\`, node, this, Comp1));
+    return toggler(Comp1, comp1({displayGrandChild: ctx['displayGrandChild']}, (Comp1).name + key + \`__1\`, node, this, Comp1));
   }
 }"
 `;

tests/components/__snapshots__/concurrency.test.ts.snap

@@ -1213,6 +1213,45 @@ exports[`delayed fiber does not get rendered if it was cancelled 4`] = `
 }"
 `;
 
+exports[`delayed render does not go through when t-component value changed 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  const comp1 = app.createComponent(null, false, false, false, true);
+  
+  return function template(ctx, node, key = \\"\\") {
+    const b2 = text(\`A\`);
+    const Comp1 = ctx['state'].component;
+    const b3 = toggler(Comp1, comp1({}, (Comp1).name + key + \`__1\`, node, this, Comp1));
+    return multi([b2, b3]);
+  }
+}"
+`;
+
+exports[`delayed render does not go through when t-component value changed 2`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    const b2 = text(\`B\`);
+    const b3 = text(ctx['state'].val);
+    return multi([b2, b3]);
+  }
+}"
+`;
+
+exports[`delayed render does not go through when t-component value changed 3`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return text(\`C\`);
+  }
+}"
+`;
+
 exports[`delayed rendering, but then initial rendering is cancelled by yet another render 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
@@ -1776,7 +1815,7 @@ exports[`t-foreach with dynamic async component 1`] = `
       let b3;
       if (ctx['arr']) {
         const Comp1 = ctx['myComp'];
-        b3 = toggler(Comp1, comp1({key: ctx['arr'][0]}, key + \`__1__\${key1}\`, node, this, Comp1));
+        b3 = toggler(Comp1, comp1({key: ctx['arr'][0]}, (Comp1).name + key + \`__1__\${key1}\`, node, this, Comp1));
       }
       c_block1[i1] = withKey(multi([b3]), key1);
     }
@@ -1810,7 +1849,7 @@ exports[`t-key on dom node having a component 1`] = `
   return function template(ctx, node, key = \\"\\") {
     const tKey_1 = ctx['key'];
     const Comp1 = ctx['myComp'];
-    const b2 = toggler(tKey_1, toggler(Comp1, comp1({key: ctx['key']}, tKey_1 + key + \`__1\`, node, this, Comp1)));
+    const b2 = toggler(tKey_1, toggler(Comp1, comp1({key: ctx['key']}, (Comp1).name + tKey_1 + key + \`__1\`, node, this, Comp1)));
     return toggler(tKey_1, block1([], [b2]));
   }
 }"
@@ -1836,7 +1875,7 @@ exports[`t-key on dynamic async component (toggler is never patched) 1`] = `
   return function template(ctx, node, key = \\"\\") {
     const tKey_1 = ctx['key'];
     const Comp1 = ctx['myComp'];
-    return toggler(tKey_1, toggler(Comp1, comp1({key: ctx['key']}, tKey_1 + key + \`__1\`, node, this, Comp1)));
+    return toggler(tKey_1, toggler(Comp1, comp1({key: ctx['key']}, (Comp1).name + tKey_1 + key + \`__1\`, node, this, Comp1)));
   }
 }"
 `;

tests/components/__snapshots__/error_handling.test.ts.snap

@@ -931,7 +931,7 @@ exports[`can catch errors catching error, rethrow, render parent  -- a main comp
   
   function slot1(ctx, node, key = \\"\\") {
     const Comp1 = ctx['cp'].Comp;
-    return toggler(Comp1, comp1({}, key + \`__1\`, node, this, Comp1));
+    return toggler(Comp1, comp1({}, (Comp1).name + key + \`__1\`, node, this, Comp1));
   }
   
   return function template(ctx, node, key = \\"\\") {
@@ -1010,7 +1010,7 @@ exports[`can catch errors catching in child makes parent render 1`] = `
   
   function slot1(ctx, node, key = \\"\\") {
     const Comp1 = ctx['elem'][1];
-    return toggler(Comp1, comp1({id: ctx['elem'][0]}, key + \`__1\`, node, this, Comp1));
+    return toggler(Comp1, comp1({id: ctx['elem'][0]}, (Comp1).name + key + \`__1\`, node, this, Comp1));
   }
   
   return function template(ctx, node, key = \\"\\") {

tests/components/__snapshots__/t_component.test.ts.snap

@@ -10,7 +10,7 @@ exports[`t-component can switch between dynamic components without the need for
   
   return function template(ctx, node, key = \\"\\") {
     const Comp1 = ctx['constructor'].components[ctx['state'].child];
-    const b2 = toggler(Comp1, comp1({}, key + \`__1\`, node, this, Comp1));
+    const b2 = toggler(Comp1, comp1({}, (Comp1).name + key + \`__1\`, node, this, Comp1));
     return block1([], [b2]);
   }
 }"
@@ -51,7 +51,7 @@ exports[`t-component can use dynamic components (the class) if given (with diffe
   return function template(ctx, node, key = \\"\\") {
     const tKey_1 = ctx['state'].child;
     const Comp1 = ctx['myComponent'];
-    return toggler(tKey_1, toggler(Comp1, comp1({}, tKey_1 + key + \`__1\`, node, this, Comp1)));
+    return toggler(tKey_1, toggler(Comp1, comp1({}, (Comp1).name + tKey_1 + key + \`__1\`, node, this, Comp1)));
   }
 }"
 `;
@@ -91,7 +91,7 @@ exports[`t-component can use dynamic components (the class) if given 1`] = `
   return function template(ctx, node, key = \\"\\") {
     const tKey_1 = ctx['state'].child;
     const Comp1 = ctx['myComponent'];
-    return toggler(tKey_1, toggler(Comp1, comp1({}, tKey_1 + key + \`__1\`, node, this, Comp1)));
+    return toggler(tKey_1, toggler(Comp1, comp1({}, (Comp1).name + tKey_1 + key + \`__1\`, node, this, Comp1)));
   }
 }"
 `;
@@ -132,7 +132,7 @@ exports[`t-component modifying a sub widget 1`] = `
   
   return function template(ctx, node, key = \\"\\") {
     const Comp1 = ctx['Counter'];
-    const b2 = toggler(Comp1, comp1({}, key + \`__1\`, node, this, Comp1));
+    const b2 = toggler(Comp1, comp1({}, (Comp1).name + key + \`__1\`, node, this, Comp1));
     return block1([], [b2]);
   }
 }"
@@ -162,7 +162,7 @@ exports[`t-component switching dynamic component 1`] = `
   
   return function template(ctx, node, key = \\"\\") {
     const Comp1 = ctx['Child'];
-    return toggler(Comp1, comp1({}, key + \`__1\`, node, this, Comp1));
+    return toggler(Comp1, comp1({}, (Comp1).name + key + \`__1\`, node, this, Comp1));
   }
 }"
 `;
@@ -199,7 +199,7 @@ exports[`t-component t-component works in simple case 1`] = `
   
   return function template(ctx, node, key = \\"\\") {
     const Comp1 = ctx['Child'];
-    return toggler(Comp1, comp1({}, key + \`__1\`, node, this, Comp1));
+    return toggler(Comp1, comp1({}, (Comp1).name + key + \`__1\`, node, this, Comp1));
   }
 }"
 `;

tests/components/concurrency.test.ts

@@ -1,6 +1,7 @@
 import {
   App,
   Component,
+  ComponentConstructor,
   mount,
   onMounted,
   onRendered,
@@ -4067,6 +4068,64 @@ test("renderings, destruction, patch, stuff, ... yet another variation", async (
   expect(fixture.innerHTML).toBe("ABD<p>2</p>");
 });
 
+test("delayed render does not go through when t-component value changed", async () => {
+  class C extends Component {
+    static template = xml`C`;
+    setup() {
+      useLogLifecycle("", true);
+    }
+  }
+
+  class B extends Component {
+    static template = xml`B<t t-esc="state.val"/>`;
+    state = useState({ val: 1 });
+    setup() {
+      useLogLifecycle("", true);
+      b = this;
+    }
+  }
+  let b: B;
+
+  class A extends Component {
+    static template = xml`A<t t-component="state.component"/>`;
+    state: { component: ComponentConstructor } = useState({ component: B });
+    setup() {
+      useLogLifecycle("", true);
+    }
+  }
+
+  const a = await mount(A, fixture);
+  expect(fixture.innerHTML).toBe("AB1");
+  expect([
+    "A:setup",
+    "A:willRender",
+    "B:setup",
+    "A:rendered",
+    "B:willRender",
+    "B:rendered",
+    "B:mounted",
+    "A:mounted",
+  ]).toBeLogged();
+  // start a render in B
+  b!.state.val = 2;
+  // start a render in A, invalidating the scheduled render of B, which could crash if executed.
+  a.state.component = C;
+  await nextTick();
+  expect(fixture.innerHTML).toBe("AC");
+  expect([
+    "A:willRender",
+    "C:setup",
+    "A:rendered",
+    "C:willRender",
+    "C:rendered",
+    "A:willPatch",
+    "B:willUnmount",
+    "B:willDestroy",
+    "C:mounted",
+    "A:patched",
+  ]).toBeLogged();
+});
+
 //   test.skip("components with shouldUpdate=false", async () => {
 //     const state = { p: 1, cc: 10 };