[FIX] component: render in delayed willUpdateProps

Have a child component on which a render is triggered.
This component delays its willUpdateProps and makes a rendering during the willUpdateProps

Before this commit, renderings of the child were inconsistent across
its parent's renderings.

After this commit, it works as expected.

src/component/component_node.ts

@@ -203,12 +203,6 @@ export class ComponentNode<T extends typeof Component = typeof Component>
     // update
     const fiber = makeChildFiber(this, parentFiber);
     this.fiber = fiber;
-    if (this.willPatch.length) {
-      parentFiber.root.willPatch.push(fiber);
-    }
-    if (this.patched.length) {
-      parentFiber.root.patched.push(fiber);
-    }
     const component = this.component;
     applyDefaultProps(props, component.constructor as any);
     const prom = Promise.all(this.willUpdateProps.map((f) => f.call(component, props)));
@@ -216,8 +210,15 @@ export class ComponentNode<T extends typeof Component = typeof Component>
     if (fiber !== this.fiber) {
       return;
     }
-    this.component.props = props;
+    component.props = props;
     this._render(fiber);
+    const parentRoot = parentFiber.root;
+    if (this.willPatch.length) {
+      parentRoot.willPatch.push(fiber);
+    }
+    if (this.patched.length) {
+      parentRoot.patched.push(fiber);
+    }
   }
 
   /**

src/component/fibers.ts

@@ -3,15 +3,44 @@ import type { ComponentNode } from "./component_node";
 import { fibersInError, handleError } from "./error_handling";
 import { STATUS } from "./status";
 
+/**
+ * Cleans on the root fiber the patch and willPatch fiber lists
+ * It is typically needed when the same root fiber needs to recycle on
+ * of its children or grandchildren's fiber.
+ */
+function cleanPatchableFiber(child: Fiber, root: RootFiber) {
+  const { willPatch, patched } = root;
+  let i = willPatch.indexOf(child);
+  if (i > -1) {
+    willPatch.splice(i, 1);
+  }
+  i = patched.indexOf(child);
+  if (i > -1) {
+    patched.splice(i, 1);
+  }
+}
+
 export function makeChildFiber(node: ComponentNode, parent: Fiber): Fiber {
   let current = node.fiber;
   if (current) {
     // current is necessarily a rootfiber here
     let root = parent.root;
+    const isSameRoot = current.root === root;
     cancelFibers(root, current.children);
     current.children = [];
     current.parent = parent;
-    root.counter++;
+    // only increment our rendering if we were not
+    // already accounted for, or that we have been rendered
+    // already (in which case our fiber was removed from the root rendering)
+    if (!isSameRoot || current.bdom) {
+      root.counter++;
+    }
+
+    if (isSameRoot) {
+      cleanPatchableFiber(current, root);
+    }
+
+    current.bdom = null;
     current.root = root;
     return current;
   }

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

@@ -939,6 +939,82 @@ exports[`creating two async components, scenario 3 (patching in the same frame)
 }"
 `;
 
+exports[`delay willUpdateProps 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b2 = text(ctx['props'].value);
+    let b3 = text(\`_\`);
+    let b4 = text(ctx['state'].int);
+    return multi([b2, b3, b4]);
+  }
+}"
+`;
+
+exports[`delay willUpdateProps 2`] = `
+"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[`delay willUpdateProps with rendering grandchild 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b2 = text(ctx['props'].value);
+    let b3 = text(\`_\`);
+    let b4 = text(ctx['state'].int);
+    return multi([b2, b3, b4]);
+  }
+}"
+`;
+
+exports[`delay willUpdateProps with rendering grandchild 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  let block1 = createBlock(\`<div/>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
+exports[`delay willUpdateProps with rendering grandchild 3`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b2 = component(\`DelayedChild\`, {value: ctx['props'].state.value}, key + \`__1\`, node, ctx);
+    let b3 = component(\`ReactiveChild\`, {}, key + \`__2\`, node, ctx);
+    return multi([b2, b3]);
+  }
+}"
+`;
+
+exports[`delay willUpdateProps with rendering grandchild 4`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return component(\`Parent\`, {state: ctx['state']}, key + \`__1\`, node, ctx);
+  }
+}"
+`;
+
 exports[`destroying/recreating a subwidget with different props (if start is not over) 1`] = `
 "function anonymous(bdom, helpers
 ) {
@@ -1099,6 +1175,28 @@ exports[`two renderings initiated between willPatch and patched 2`] = `
 }"
 `;
 
+exports[`two sequential renderings before an animation frame 1`] = `
+"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[`two sequential renderings before an animation frame 2`] = `
+"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[`update a sub-component twice in the same frame 1`] = `
 "function anonymous(bdom, helpers
 ) {

tests/components/concurrency.test.ts

@@ -1590,12 +1590,12 @@ test("concurrent renderings scenario 9", async () => {
     "ComponentB:willRender",
     "ComponentB:rendered",
     "ComponentA:willPatch",
-    "ComponentB:willPatch",
     "ComponentC:willPatch",
     "ComponentD:willPatch",
+    "ComponentB:willPatch",
+    "ComponentB:patched",
     "ComponentD:patched",
     "ComponentC:patched",
-    "ComponentB:patched",
     "ComponentA:patched",
   ]).toBeLogged();
 });
@@ -2583,6 +2583,292 @@ test("parent and child rendered at exact same time", async () => {
   ]).toBeLogged();
 });
 
+test("delay willUpdateProps", async () => {
+  let promise: any = null;
+  let child: any;
+
+  class Child extends Component {
+    static template = xml`<t t-esc="props.value"/>_<t t-esc="state.int" />`;
+    state: any;
+    setup() {
+      useLogLifecycle();
+      child = this;
+      this.state = useState({ int: 0 });
+      onWillUpdateProps(async () => {
+        await promise;
+        this.state.int++;
+      });
+    }
+  }
+
+  class Parent extends Component {
+    static template = xml`<Child value="state.value"/>`;
+    static components = { Child };
+    state = { value: 0 };
+    setup() {
+      useLogLifecycle();
+    }
+  }
+  const parent = await mount(Parent, fixture);
+  expect(fixture.innerHTML).toBe("0_0");
+  expect([
+    "Parent:setup",
+    "Parent:willStart",
+    "Parent:willRender",
+    "Child:setup",
+    "Child:willStart",
+    "Parent:rendered",
+    "Child:willRender",
+    "Child:rendered",
+    "Child:mounted",
+    "Parent:mounted",
+  ]).toBeLogged();
+
+  promise = makeDeferred();
+  const prom1 = promise;
+  parent.state.value = 1;
+  child.render(); // trigger a root rendering first
+  parent.render();
+  await nextTick();
+  expect(fixture.innerHTML).toBe("0_0");
+  expect([
+    "Child:willRender",
+    "Child:rendered",
+    "Parent:willRender",
+    "Child:willUpdateProps",
+    "Parent:rendered",
+  ]).toBeLogged();
+
+  promise = makeDeferred();
+  const prom2 = promise;
+  parent.state.value = 2;
+  parent.render();
+  await nextTick();
+  expect(fixture.innerHTML).toBe("0_0");
+
+  prom2.resolve();
+  await nextTick();
+  expect(fixture.innerHTML).toBe("2_1");
+
+  expect([
+    "Parent:willRender",
+    "Child:willUpdateProps",
+    "Parent:rendered",
+    "Child:willRender",
+    "Child:rendered",
+    "Parent:willPatch",
+    "Child:willPatch",
+    "Child:patched",
+    "Parent:patched",
+  ]).toBeLogged();
+
+  prom1.resolve();
+  await nextTick();
+  expect(fixture.innerHTML).toBe("2_2");
+
+  expect(["Child:willRender", "Child:rendered", "Child:willPatch", "Child:patched"]).toBeLogged();
+});
+
+test("delay willUpdateProps with rendering grandchild", async () => {
+  // This test is a bit tricky, a Parent and one of his grandchildren render while another of the parent's
+  // grandchildren is awaiting its willUpdateProps.
+  // Technically RootFibers will be downgraded in ChildFibers, keeping the same container RootFiber.
+  // This case happens when Parent and ReaciveChild react together to a change in a reactive state/
+  let promise: any = null;
+  let child: any;
+  let reactiveChild: any;
+
+  // Delayed willUpdateProps
+  class DelayedChild extends Component {
+    static template = xml`<t t-esc="props.value"/>_<t t-esc="state.int" />`;
+    state: any;
+    setup() {
+      useLogLifecycle();
+      child = this;
+      this.state = useState({ int: 0 });
+      onWillUpdateProps(async () => {
+        await promise;
+        this.state.int++;
+      });
+    }
+  }
+
+  // A sibling of the delayed component, we will trigger a render on it manually
+  class ReactiveChild extends Component {
+    static template = xml`<div />`;
+    setup() {
+      reactiveChild = this;
+      useLogLifecycle();
+    }
+  }
+
+  // The parent of everybody, we also manually trigger render on it.
+  class Parent extends Component {
+    static template = xml`<DelayedChild value="props.state.value"/><ReactiveChild />`;
+    static components = { DelayedChild, ReactiveChild };
+    setup() {
+      useLogLifecycle();
+    }
+  }
+
+  class GrandParent extends Component {
+    static template = xml`<Parent state="state"/>`;
+    static components = { Parent };
+    state = { value: 0 };
+  }
+  const parent = await mount(GrandParent, fixture);
+  expect(fixture.innerHTML).toBe("0_0<div></div>");
+  expect([
+    "Parent:setup",
+    "Parent:willStart",
+    "Parent:willRender",
+    "DelayedChild:setup",
+    "DelayedChild:willStart",
+    "ReactiveChild:setup",
+    "ReactiveChild:willStart",
+    "Parent:rendered",
+    "DelayedChild:willRender",
+    "DelayedChild:rendered",
+    "ReactiveChild:willRender",
+    "ReactiveChild:rendered",
+    "ReactiveChild:mounted",
+    "DelayedChild:mounted",
+    "Parent:mounted",
+  ]).toBeLogged();
+
+  promise = makeDeferred();
+  const prom1 = promise;
+  parent.state.value = 1;
+  child.render(); // trigger a root rendering first
+  parent.render();
+  reactiveChild.render();
+  await nextTick();
+  expect(fixture.innerHTML).toBe("0_0<div></div>");
+  expect([
+    "DelayedChild:willRender",
+    "DelayedChild:rendered",
+    "Parent:willUpdateProps",
+    "ReactiveChild:willRender",
+    "ReactiveChild:rendered",
+    "Parent:willRender",
+    "DelayedChild:willUpdateProps",
+    "ReactiveChild:willUpdateProps",
+    "Parent:rendered",
+    "ReactiveChild:willRender",
+    "ReactiveChild:rendered",
+  ]).toBeLogged();
+
+  promise = makeDeferred();
+  const prom2 = promise;
+  child.render(); // trigger a root rendering first
+  parent.state.value = 2;
+  parent.render();
+  reactiveChild.render();
+  await nextTick();
+  expect(fixture.innerHTML).toBe("0_0<div></div>");
+  expect([
+    "Parent:willUpdateProps",
+    "ReactiveChild:willRender",
+    "ReactiveChild:rendered",
+    "Parent:willRender",
+    "DelayedChild:willUpdateProps",
+    "ReactiveChild:willUpdateProps",
+    "Parent:rendered",
+    "ReactiveChild:willRender",
+    "ReactiveChild:rendered",
+  ]).toBeLogged();
+
+  prom2.resolve();
+  await nextTick();
+  expect(fixture.innerHTML).toBe("2_1<div></div>");
+  expect([
+    "DelayedChild:willRender",
+    "DelayedChild:rendered",
+    "Parent:willPatch",
+    "ReactiveChild:willPatch",
+    "DelayedChild:willPatch",
+    "DelayedChild:patched",
+    "ReactiveChild:patched",
+    "Parent:patched",
+  ]).toBeLogged();
+
+  prom1.resolve();
+  await nextTick();
+  expect(fixture.innerHTML).toBe("2_2<div></div>");
+  expect([
+    "DelayedChild:willRender",
+    "DelayedChild:rendered",
+    "DelayedChild:willPatch",
+    "DelayedChild:patched",
+  ]).toBeLogged();
+});
+
+test("two sequential renderings before an animation frame", 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: 0 });
+    setup() {
+      useLogLifecycle();
+    }
+  }
+  const parent = await mount(Parent, fixture);
+  expect(fixture.innerHTML).toBe("0");
+  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 = 1;
+  await nextMicroTick();
+  await nextMicroTick();
+  await nextMicroTick();
+  await nextMicroTick();
+  await nextMicroTick();
+  expect(fixture.innerHTML).toBe("0");
+  expect([
+    "Parent:willRender",
+    "Child:willUpdateProps",
+    "Parent:rendered",
+    "Child:willRender",
+    "Child:rendered",
+  ]).toBeLogged();
+
+  parent.state.value = 2;
+  // enough microticks to wait for render + willupdateprops
+  await nextMicroTick();
+  await nextMicroTick();
+  await nextMicroTick();
+  await nextMicroTick();
+  await nextMicroTick();
+  expect(fixture.innerHTML).toBe("0");
+  expect([
+    "Parent:willRender",
+    "Child:willUpdateProps",
+    "Parent:rendered",
+    "Child:willRender",
+    "Child:rendered",
+  ]).toBeLogged();
+
+  await nextTick();
+  // we check here that the willPatch and patched hooks are called only once
+  expect(["Parent:willPatch", "Child:willPatch", "Child:patched", "Parent:patched"]).toBeLogged();
+});
 //   test.skip("components with shouldUpdate=false", async () => {
 //     const state = { p: 1, cc: 10 };