[FIX] component: fiber, don't render a destroyed component

A little typo was preventing a dynamic t-slot with a scope
(`<t t-slot="{{ state.name }}" myScope="someValue" />`)
to work properly.

This commit corrects this.

src/compiler/code_generator.ts

@@ -1234,7 +1234,7 @@ export class CodeGenerator {
       if (dynamic) {
         let name = this.generateId("slot");
         this.define(name, slotName);
-        blockString = `toggler(${name}, callSlot(ctx, node, key, ${name}), ${dynamic}, ${scope})`;
+        blockString = `toggler(${name}, callSlot(ctx, node, key, ${name}, ${dynamic}, ${scope}))`;
       } else {
         blockString = `callSlot(ctx, node, key, ${slotName}, ${dynamic}, ${scope})`;
       }

src/component/component_node.ts

@@ -116,9 +116,10 @@ export function component<P extends object>(
     }
     node = new ComponentNode(C, props, ctx.app, ctx);
     ctx.children[key] = node;
-
-    node.initiateRender(makeChildFiber(node, parentFiber));
+    node.initiateRender(new Fiber(node, parentFiber));
   }
+  parentFiber.childrenMap[key] = node;
+
   parentFiber.root!.reachedChildren.add(node);
   return node;
 }
@@ -326,6 +327,7 @@ export class ComponentNode<P extends object = any, E = any> implements VNode<Com
     bdom.mount(parent, anchor);
     this.status = STATUS.MOUNTED;
     this.fiber!.appliedToDom = true;
+    this.children = this.fiber!.childrenMap;
     this.fiber = null;
   }
 
@@ -334,7 +336,6 @@ export class ComponentNode<P extends object = any, E = any> implements VNode<Com
   }
 
   patch() {
-    debugger
     if (this.fiber && this.fiber.parent) {
       // we only patch here renderings coming from above. renderings initiated
       // by the component will be patched independently in the appropriate
@@ -344,10 +345,8 @@ export class ComponentNode<P extends object = any, E = any> implements VNode<Com
   }
   _patch() {
     const hasChildren = Object.keys(this.children).length > 0;
+    this.children = this.fiber!.childrenMap;
     this.bdom!.patch(this!.fiber!.bdom!, hasChildren);
-    if (hasChildren) {
-      this.cleanOutdatedChildren();
-    }
     this.fiber!.appliedToDom = true;
     this.fiber = null;
   }
@@ -360,20 +359,6 @@ export class ComponentNode<P extends object = any, E = any> implements VNode<Com
     this.bdom!.remove();
   }
 
-  cleanOutdatedChildren() {
-    const children = this.children;
-    for (const key in children) {
-      const node = children[key];
-      const status = node.status;
-      if (status !== STATUS.MOUNTED) {
-        delete children[key];
-        if (status !== STATUS.DESTROYED) {
-          node.destroy();
-        }
-      }
-    }
-  }
-
   // ---------------------------------------------------------------------------
   // Some debug helpers
   // ---------------------------------------------------------------------------

src/component/fibers.ts

@@ -1,6 +1,6 @@
 import { BDom, mount } from "../blockdom";
 import type { ComponentNode } from "./component_node";
-import { handleError } from "./error_handling";
+import { fibersInError, handleError } from "./error_handling";
 import { STATUS } from "./status";
 
 export function makeChildFiber(node: ComponentNode, parent: Fiber): Fiber {
@@ -9,72 +9,28 @@ export function makeChildFiber(node: ComponentNode, parent: Fiber): Fiber {
     cancelFibers(current.children);
     current.root = null;
   }
-  let fiber = new Fiber(node);
-  fiber.deep = parent.deep;
-  const root = parent.root!;
-  fiber.root = root;
-  parent.children.push(fiber);
-  fiber.parent = parent;
-  root.setCounter(root.counter + 1);
-  return fiber;
+  return new Fiber(node, parent);
 }
 
 export function makeRootFiber(node: ComponentNode): Fiber {
   let current = node.fiber;
   if (current) {
-    debugger;
-    let parent = current.parent;
-    let n = cancelFibers(current.children);
-    current.root = null;
-    // current.bdom = null;
-    if (parent) {
-      let fiber = new Fiber(node);
-      fiber.deep = parent.deep;
-      const root = parent.root!;
-      fiber.root = root;
-      let index = parent.children.indexOf(current);
-      parent.children[index] = fiber;
-      // parent.children.push(fiber);
-      fiber.parent = parent;
-      // node.fiber = 
-      root.setCounter(root.counter + 1 - n);
-      if (node.willPatch.length) {
-        let index = root.willPatch.indexOf(current);
-        if (index >= 0) {
-          root.willPatch[index] = fiber;
-        }
-      }
-      if (node.patched.length) {
-        let index = root.patched.indexOf(current);
-        if (index >= 0) {
-          root.patched[index] = fiber;
-        }
-      }
-    
-      return fiber;
+    let root = current.root!;
+    root.setCounter(root.counter + 1 - cancelFibers(current.children));
+    current.children = [];
+    current.childrenMap = {};
+    current.bdom = null;
+    if (current === root) {
+      root.reachedChildren = new WeakSet();
     }
-    // let parent = current.parent;
-    // let fiber = new Fiber(node);
-    // return fiber;
-
-    // cancelFibers(current.children);
-    // current.root = null;
-    // let root = current.root!;
-    // root.setCounter(root.counter + 1 - cancelFibers(current.children));
-    // current.children = [];
-    // current.bdom = null;
-    // if (current === root) {
-    //   root.reachedChildren = new WeakSet();
-    // }
-    // if (fibersInError.has(current)) {
-    //   fibersInError.delete(current);
-    //   fibersInError.delete(root);
-    //   current.appliedToDom = false;
-    // }
-    // return current;
+    if (fibersInError.has(current)) {
+      fibersInError.delete(current);
+      fibersInError.delete(root);
+      current.appliedToDom = false;
+    }
+    return current;
   }
-  const fiber = new RootFiber(node);
-  fiber.root = fiber;
+  const fiber = new RootFiber(node, null);
   if (node.willPatch.length) {
     fiber.willPatch.push(fiber);
   }
@@ -90,7 +46,11 @@ export function makeRootFiber(node: ComponentNode): Fiber {
 function cancelFibers(fibers: Fiber[]): number {
   let result = 0;
   for (let fiber of fibers) {
-    fiber.node.fiber = null;
+    let node = fiber.node;
+    if (node.status === STATUS.NEW) {
+      node.destroy();
+    }
+    node.fiber = null;
     if (fiber.bdom) {
       // if fiber has been rendered, this means that the component props have
       // been updated. however, this fiber will not be patched to the dom, so
@@ -98,7 +58,7 @@ function cancelFibers(fibers: Fiber[]): number {
       // the same props, and skip the render completely. With the next line,
       // we kindly request the component code to force a render, so it works as
       // expected.
-      fiber.node.forceNextRender = true;
+      node.forceNextRender = true;
     } else {
       result++;
     }
@@ -107,20 +67,28 @@ function cancelFibers(fibers: Fiber[]): number {
   return result;
 }
 
-(window as any).fibers = [];
-
 export class Fiber {
   node: ComponentNode;
   bdom: BDom | null = null;
-  root: RootFiber | null = null; // A Fiber that has been replaced by another has no root
-  parent: Fiber | null = null;
+  root: RootFiber | null; // A Fiber that has been replaced by another has no root
+  parent: Fiber | null;
   children: Fiber[] = [];
   appliedToDom = false;
   deep: boolean = false;
+  childrenMap: ComponentNode["children"] = {};
 
-  constructor(node: ComponentNode) {
+  constructor(node: ComponentNode, parent: Fiber | null) {
     this.node = node;
-    (window as any).fibers.push(this);
+    this.parent = parent;
+    if (parent) {
+      this.deep = parent.deep;
+      const root = parent.root!;
+      root.setCounter(root.counter + 1);
+      this.root = root;
+      parent.children.push(this);
+    } else {
+      this.root = this as any;
+    }
   }
 
   render() {
@@ -153,6 +121,9 @@ export class Fiber {
 
   _render() {
     const node = this.node;
+    if (node.status > STATUS.MOUNTED) {
+      return;
+    }
     const root = this.root;
     if (root) {
       try {
@@ -230,7 +201,6 @@ export class RootFiber extends Fiber {
   }
 
   setCounter(newValue: number) {
-    debugger;
     this.counter = newValue;
     if (newValue === 0) {
       this.node.app.scheduler.flush();
@@ -249,15 +219,15 @@ export class MountFiber extends RootFiber {
   position: Position;
 
   constructor(node: ComponentNode, target: HTMLElement, options: MountOptions = {}) {
-    super(node);
+    super(node, null);
     this.target = target;
-    this.root = this;
     this.position = options.position || "last-child";
   }
   complete() {
     let current: Fiber | undefined = this;
     try {
       const node = this.node;
+      node.children = this.childrenMap;
       (node.app.constructor as any).validateTarget(this.target);
       if (node.bdom) {
         // this is a complicated situation: if we mount a fiber with an existing

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

@@ -626,7 +626,7 @@ exports[`slots dynamic t-slot call 2`] = `
   return function template(ctx, node, key = \\"\\") {
     let hdlr1 = [ctx['toggle'], ctx];
     const slot1 = (ctx['current'].slot);
-    const b2 = toggler(slot1, callSlot(ctx, node, key, slot1), true, null);
+    const b2 = toggler(slot1, callSlot(ctx, node, key, slot1, true, null));
     return block1([hdlr1], [b2]);
   }
 }"
@@ -1444,6 +1444,45 @@ exports[`slots simple default slot, variation 2`] = `
 }"
 `;
 
+exports[`slots simple dynamic slot with slot scope 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  let { capture, markRaw } = helpers;
+  
+  function slot1(ctx, node, key = \\"\\") {
+    let b2,b3;
+    if (ctx['slotScope'].bool) {
+      b2 = text(\`some text\`);
+    } else {
+      b3 = text(\`other text\`);
+    }
+    return multi([b2, b3]);
+  }
+  
+  return function template(ctx, node, key = \\"\\") {
+    const ctx1 = capture(ctx);
+    return component(\`Child\`, {slots: markRaw({'slotName': {__render: slot1, __ctx: ctx1, __scope: \\"slotScope\\"}})}, key + \`__1\`, node, ctx);
+  }
+}"
+`;
+
+exports[`slots simple dynamic slot with slot scope 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  let { callSlot } = helpers;
+  
+  let block1 = createBlock(\`<span><block-child-0/></span>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    const slot1 = ('slotName');
+    const b2 = toggler(slot1, callSlot(ctx, node, key, slot1, true, {bool: ctx['state'].bool}));
+    return block1([], [b2]);
+  }
+}"
+`;
+
 exports[`slots simple named and empty slot -- 2 1`] = `
 "function anonymous(bdom, helpers
 ) {
@@ -1754,7 +1793,7 @@ exports[`slots slot content has different key from other content -- dynamic slot
   return function template(ctx, node, key = \\"\\") {
     const b2 = component(\`Child\`, {parent: 'SlotDisplay'}, key + \`__1\`, node, ctx);
     const slot1 = (ctx['slotName']);
-    const b3 = toggler(slot1, callSlot(ctx, node, key, slot1), true, null);
+    const b3 = toggler(slot1, callSlot(ctx, node, key, slot1, true, null));
     return multi([b2, b3]);
   }
 }"

tests/components/concurrency.test.ts

@@ -114,8 +114,8 @@ test("destroying/recreating a subwidget with different props (if start is not ov
   expect(n).toBe(2);
 
   expect([
-    "W:willRender",
     "Child:willDestroy",
+    "W:willRender",
     "Child:setup",
     "Child:willStart",
     "W:rendered",
@@ -244,8 +244,8 @@ test("creating two async components, scenario 1", async () => {
   await nextTick();
   expect(fixture.innerHTML).toBe("");
   expect([
-    "Parent:willRender",
     "ChildA:willDestroy",
+    "Parent:willRender",
     "ChildA:setup",
     "ChildA:willStart",
     "ChildB:setup",
@@ -696,8 +696,8 @@ test("rendering component again in next microtick", async () => {
     "Child:setup",
     "Child:willStart",
     "Parent:rendered",
-    "Parent:willRender",
     "Child:willDestroy",
+    "Parent:willRender",
     "Child:setup",
     "Child:willStart",
     "Parent:rendered",
@@ -870,7 +870,6 @@ test("concurrent renderings scenario 2", async () => {
     "ComponentB:rendered",
   ]).toBeLogged();
 
-  debugger; 
   stateB.fromB = "c";
   await nextTick();
   expect(fixture.innerHTML).toBe("<div>1<p><span>1b</span></p></div>");
@@ -882,7 +881,7 @@ test("concurrent renderings scenario 2", async () => {
 
   defs[1].resolve(); // resolve rendering initiated in B
   await nextTick();
-  debugger
+  expect(fixture.innerHTML).toBe("<div>2<p><span>2c</span></p></div>");
   expect([
     "ComponentC:willRender",
     "ComponentC:rendered",
@@ -893,7 +892,6 @@ test("concurrent renderings scenario 2", async () => {
     "ComponentB:patched",
     "ComponentA:patched",
   ]).toBeLogged();
-  expect(fixture.innerHTML).toBe("<div>2<p><span>2c</span></p></div>");
 
   defs[0].resolve(); // resolve rendering initiated in A
   await nextTick();
@@ -1727,6 +1725,7 @@ test("concurrent renderings scenario 10", async () => {
   expect(fixture.innerHTML).toBe("<div><p></p></div>");
   expect([
     "ComponentA:willRender",
+    "ComponentC:willDestroy",
     "ComponentB:willUpdateProps",
     "ComponentA:rendered",
   ]).toBeLogged();
@@ -1737,7 +1736,6 @@ test("concurrent renderings scenario 10", async () => {
   expect(rendered).toBe(1);
   expect([
     "ComponentB:willRender",
-    "ComponentC:willDestroy",
     "ComponentC:setup",
     "ComponentC:willStart",
     "ComponentB:rendered",
@@ -2277,11 +2275,11 @@ test("concurrent renderings scenario 16", async () => {
     "D:setup",
     "D:willStart",
     "C:rendered",
+    "D:willDestroy",
     "B:willRender",
     "C:willUpdateProps",
     "B:rendered",
     "C:willRender",
-    "D:willDestroy",
     "D:setup",
     "D:willStart",
     "C:rendered",
@@ -2992,13 +2990,13 @@ test("t-key on dom node having a component", async () => {
 
   expect(fixture.innerHTML).toBe("<div>3</div>");
   expect([
+    "Child (2):willDestroy",
     "Child (3):setup",
     "Child (3):willStart",
     "Child (3):willRender",
     "Child (3):rendered",
     "Child (1):willUnmount",
     "Child (1):willDestroy",
-    "Child (2):willDestroy",
     "Child (3):mounted",
   ]).toBeLogged();
 });
@@ -3050,13 +3048,13 @@ test("t-key on dynamic async component (toggler is never patched)", async () =>
 
   expect(fixture.innerHTML).toBe("<div>3</div>");
   expect([
+    "Child (2):willDestroy",
     "Child (3):setup",
     "Child (3):willStart",
     "Child (3):willRender",
     "Child (3):rendered",
     "Child (1):willUnmount",
     "Child (1):willDestroy",
-    "Child (2):willDestroy",
     "Child (3):mounted",
   ]).toBeLogged();
 });
@@ -3109,13 +3107,13 @@ test("t-foreach with dynamic async component", async () => {
 
   expect(fixture.innerHTML).toBe("<div>3</div>");
   expect([
+    "Child (2):willDestroy",
     "Child (3):setup",
     "Child (3):willStart",
     "Child (3):willRender",
     "Child (3):rendered",
     "Child (1):willUnmount",
     "Child (1):willDestroy",
-    "Child (2):willDestroy",
     "Child (3):mounted",
   ]).toBeLogged();
 });

tests/components/slots.test.ts

@@ -74,6 +74,35 @@ describe("slots", () => {
     expect(fixture.innerHTML).toBe("<span>other text</span>");
   });
 
+  test("simple dynamic slot with slot scope", async () => {
+    let child: any;
+    class Child extends Component {
+      static template = xml`<span><t t-slot="{{ 'slotName' }}" bool="state.bool"/></span>`;
+      state = useState({ bool: true });
+      setup() {
+        child = this;
+      }
+    }
+
+    class Parent extends Component {
+      static template = xml`
+        <Child>
+          <t t-set-slot="slotName" t-slot-scope="slotScope">
+            <t t-if="slotScope.bool">some text</t>
+            <t t-else="slotScope.bool">other text</t>
+          </t>
+        </Child>`;
+      static components = { Child };
+    }
+
+    await mount(Parent, fixture);
+    expect(fixture.innerHTML).toBe("<span>some text</span>");
+
+    child.state.bool = false;
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<span>other text</span>");
+  });
+
   test("simple named and empty slot", async () => {
     class Child extends Component {
       static template = xml`<span><t t-slot="default" /><t t-slot="myEmptySlot"/></span>`;