From 989b0d6709255c4934238cd1b7c9190ffb644db6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= <ged@odoo.com>
Date: Wed, 30 Mar 2022 16:10:04 +0200
Subject: [PATCH] [REF] improve children handling in components/fibers

---
 src/component/component_node.ts      | 22 ++++------------------
 src/component/fibers.ts              | 11 +++++++++--
 tests/components/concurrency.test.ts | 16 ++++++++--------
 3 files changed, 21 insertions(+), 28 deletions(-)

diff --git a/src/component/component_node.ts b/src/component/component_node.ts
index 5f7c3a12..656d6727 100644
--- a/src/component/component_node.ts
+++ b/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(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;
   }
 
@@ -343,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;
   }
@@ -359,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
   // ---------------------------------------------------------------------------
diff --git a/src/component/fibers.ts b/src/component/fibers.ts
index bb071d39..169e02dd 100644
--- a/src/component/fibers.ts
+++ b/src/component/fibers.ts
@@ -18,6 +18,7 @@ export function makeRootFiber(node: ComponentNode): 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();
@@ -45,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
@@ -53,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++;
     }
@@ -70,6 +75,7 @@ export class Fiber {
   children: Fiber[] = [];
   appliedToDom = false;
   deep: boolean = false;
+  childrenMap: ComponentNode["children"] = {};
 
   constructor(node: ComponentNode, parent: Fiber | null) {
     this.node = node;
@@ -218,6 +224,7 @@ export class MountFiber extends RootFiber {
     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
diff --git a/tests/components/concurrency.test.ts b/tests/components/concurrency.test.ts
index daefa896..dbf2b3c4 100644
--- a/tests/components/concurrency.test.ts
+++ b/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",
@@ -1725,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();
@@ -1735,7 +1736,6 @@ test("concurrent renderings scenario 10", async () => {
   expect(rendered).toBe(1);
   expect([
     "ComponentB:willRender",
-    "ComponentC:willDestroy",
     "ComponentC:setup",
     "ComponentC:willStart",
     "ComponentB:rendered",
@@ -2275,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",
@@ -2990,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();
 });
@@ -3048,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();
 });
@@ -3107,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();
 });