From 0457e5d4ed526a2cceb052310e66e1bd8064e733 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= <ged@odoo.com>
Date: Thu, 3 Mar 2022 13:57:29 +0100
Subject: [PATCH] [FIX] component: missing renderings in some cases

---
 src/component/component_node.ts               | 11 +++-
 src/component/fibers.ts                       | 10 ++-
 .../__snapshots__/concurrency.test.ts.snap    | 22 +++++++
 tests/components/concurrency.test.ts          | 65 +++++++++++++++++++
 4 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/src/component/component_node.ts b/src/component/component_node.ts
index cf1c587d..edcea9f2 100644
--- a/src/component/component_node.ts
+++ b/src/component/component_node.ts
@@ -107,8 +107,14 @@ export function component<P extends object>(
 
   const parentFiber = ctx.fiber!;
   if (node) {
-    const currentProps = node.component.props[TARGET];
-    if (parentFiber.deep || arePropsDifferent(currentProps, props)) {
+    let shouldRender = node.forceNextRender;
+    if (shouldRender) {
+      node.forceNextRender = false;
+    } else {
+      const currentProps = node.component.props[TARGET];
+      shouldRender = parentFiber.deep || arePropsDifferent(currentProps, props);
+    }
+    if (shouldRender) {
       node.updateAndRender(props, parentFiber);
     }
   } else {
@@ -143,6 +149,7 @@ export class ComponentNode<P extends object = any, E = any> implements VNode<Com
   component: Component<P, E>;
   bdom: BDom | null = null;
   status: STATUS = STATUS.NEW;
+  forceNextRender: boolean = false;
 
   renderFn: Function;
   parent: ComponentNode | null;
diff --git a/src/component/fibers.ts b/src/component/fibers.ts
index 47d95b64..487d9e4f 100644
--- a/src/component/fibers.ts
+++ b/src/component/fibers.ts
@@ -43,7 +43,15 @@ function cancelFibers(fibers: Fiber[]): number {
   let result = 0;
   for (let fiber of fibers) {
     fiber.node.fiber = null;
-    if (!fiber.bdom) {
+    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
+      // it could happen that the next render compare the current props with
+      // 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;
+    } else {
       result++;
     }
     result += cancelFibers(fiber.children);
diff --git a/tests/components/__snapshots__/concurrency.test.ts.snap b/tests/components/__snapshots__/concurrency.test.ts.snap
index 03fb8164..6aefcdcd 100644
--- a/tests/components/__snapshots__/concurrency.test.ts.snap
+++ b/tests/components/__snapshots__/concurrency.test.ts.snap
@@ -1222,6 +1222,28 @@ exports[`rendering component again in next microtick 2`] = `
 }"
 `;
 
+exports[`rendering parent twice, with different props on child and stuff 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return component(\`Child\`, {value: ctx['state'].value}, key + \`__1\`, node, ctx);
+  }
+}"
+`;
+
+exports[`rendering parent twice, with different props on child and stuff 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return text(ctx['props'].value);
+  }
+}"
+`;
+
 exports[`t-foreach with dynamic async component 1`] = `
 "function anonymous(bdom, helpers
 ) {
diff --git a/tests/components/concurrency.test.ts b/tests/components/concurrency.test.ts
index 106265ba..2525fd8f 100644
--- a/tests/components/concurrency.test.ts
+++ b/tests/components/concurrency.test.ts
@@ -3189,6 +3189,71 @@ test("Cascading renders after microtaskTick", async () => {
   await nextTick();
   expect(fixture.innerHTML).toBe("0123 _ 0123");
 });
+
+test("rendering parent twice, with different props on child and stuff", async () => {
+  class Child extends Component {
+    static template = xml`<t t-esc="props.value"/>`;
+    setup() {
+      useLogLifecycle();
+    }
+  }
+
+  class Parent extends Component {
+    static template = xml`<Child value="state.value"/>`;
+    static components = { Child };
+    state = useState({ value: 1 });
+    setup() {
+      useLogLifecycle();
+    }
+  }
+
+  const parent = await mount(Parent, fixture);
+  expect(fixture.innerHTML).toBe("1");
+  expect([
+    "Parent:setup",
+    "Parent:willStart",
+    "Parent:willRender",
+    "Child:setup",
+    "Child:willStart",
+    "Parent:rendered",
+    "Child:willRender",
+    "Child:rendered",
+    "Child:mounted",
+    "Parent:mounted",
+  ]).toBeLogged();
+
+  parent.state.value = 2;
+  // wait for child to be rendered
+  await nextMicroTick();
+  await nextMicroTick();
+  await nextMicroTick();
+  await nextMicroTick();
+  expect([
+    "Parent:willRender",
+    "Child:willUpdateProps",
+    "Parent:rendered",
+    "Child:willRender",
+    "Child:rendered",
+  ]).toBeLogged();
+  expect(fixture.innerHTML).toBe("1");
+
+  // trigger a render, but keep the props for child the same
+  parent.render();
+  await nextTick();
+  expect(fixture.innerHTML).toBe("2");
+  expect([
+    "Parent:willRender",
+    "Child:willUpdateProps",
+    "Parent:rendered",
+    "Child:willRender",
+    "Child:rendered",
+    "Parent:willPatch",
+    "Child:willPatch",
+    "Child:patched",
+    "Parent:patched",
+  ]).toBeLogged();
+});
+
 //   test.skip("components with shouldUpdate=false", async () => {
 //     const state = { p: 1, cc: 10 };