From 4cd1df46b30c302576e6b453cf363680d94fadb3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= <ged@odoo.com>
Date: Thu, 31 Mar 2022 14:28:41 +0200
Subject: [PATCH] Revert "[REF] improve children handling in components/fibers"

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

diff --git a/src/component/component_node.ts b/src/component/component_node.ts
index 656d6727..5f7c3a12 100644
--- a/src/component/component_node.ts
+++ b/src/component/component_node.ts
@@ -116,10 +116,9 @@ 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;
 }
@@ -327,7 +326,6 @@ 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;
   }
 
@@ -345,8 +343,10 @@ 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,6 +359,20 @@ 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 169e02dd..bb071d39 100644
--- a/src/component/fibers.ts
+++ b/src/component/fibers.ts
@@ -18,7 +18,6 @@ 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();
@@ -46,11 +45,7 @@ export function makeRootFiber(node: ComponentNode): Fiber {
 function cancelFibers(fibers: Fiber[]): number {
   let result = 0;
   for (let fiber of fibers) {
-    let node = fiber.node;
-    if (node.status === STATUS.NEW) {
-      node.destroy();
-    }
-    node.fiber = null;
+    fiber.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
@@ -58,7 +53,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.
-      node.forceNextRender = true;
+      fiber.node.forceNextRender = true;
     } else {
       result++;
     }
@@ -75,7 +70,6 @@ export class Fiber {
   children: Fiber[] = [];
   appliedToDom = false;
   deep: boolean = false;
-  childrenMap: ComponentNode["children"] = {};
 
   constructor(node: ComponentNode, parent: Fiber | null) {
     this.node = node;
@@ -224,7 +218,6 @@ 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 dbf2b3c4..daefa896 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([
-    "Child:willDestroy",
     "W:willRender",
+    "Child:willDestroy",
     "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([
-    "ChildA:willDestroy",
     "Parent:willRender",
+    "ChildA:willDestroy",
     "ChildA:setup",
     "ChildA:willStart",
     "ChildB:setup",
@@ -696,8 +696,8 @@ test("rendering component again in next microtick", async () => {
     "Child:setup",
     "Child:willStart",
     "Parent:rendered",
-    "Child:willDestroy",
     "Parent:willRender",
+    "Child:willDestroy",
     "Child:setup",
     "Child:willStart",
     "Parent:rendered",
@@ -1725,7 +1725,6 @@ test("concurrent renderings scenario 10", async () => {
   expect(fixture.innerHTML).toBe("<div><p></p></div>");
   expect([
     "ComponentA:willRender",
-    "ComponentC:willDestroy",
     "ComponentB:willUpdateProps",
     "ComponentA:rendered",
   ]).toBeLogged();
@@ -1736,6 +1735,7 @@ 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();
 });