From 31b57cbb40f68f0cb65e766541b230f3e2d3b20b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= <ged@odoo.com>
Date: Fri, 3 Jun 2022 15:35:04 +0200
Subject: [PATCH] [FIX] component: fix props comparison code

Before this commit, Owl had a bad interaction with the static
defaultProps code. The props comparison would be done with the new props
object (unmodified) and the current props object (with default props
applied), so the comparison would always return false, which in turn,
causes additional useless renderings.

This commit modifies component node to keep a reference to the
(unmodified) props object so we can compare it as expected.
---
 src/runtime/component_node.ts                 | 20 +++++---
 .../__snapshots__/rendering.test.ts.snap      | 24 +++++++++
 tests/components/rendering.test.ts            | 49 +++++++++++++++++++
 3 files changed, 85 insertions(+), 8 deletions(-)

diff --git a/src/runtime/component_node.ts b/src/runtime/component_node.ts
index 911eeccf..d020e475 100644
--- a/src/runtime/component_node.ts
+++ b/src/runtime/component_node.ts
@@ -30,15 +30,15 @@ export function useComponent(): Component {
 
 /**
  * Apply default props (only top level).
- *
- * Note that this method does modify in place the props
  */
-function applyDefaultProps<P>(props: P, defaultProps: Partial<P>) {
+function applyDefaultProps<P extends object>(props: P, defaultProps: Partial<P>): P {
+  const result = Object.create(props);
   for (let propName in defaultProps) {
-    if ((props as any)[propName] === undefined) {
-      (props as any)[propName] = defaultProps[propName];
+    if (props[propName] === undefined) {
+      result[propName] = defaultProps[propName];
     }
   }
+  return result;
 }
 // -----------------------------------------------------------------------------
 // Integration with reactivity system (useState)
@@ -106,7 +106,7 @@ export function component<P extends Props>(
     if (shouldRender) {
       node.forceNextRender = false;
     } else {
-      const currentProps = node.component.props;
+      const currentProps = node.props;
       shouldRender = parentFiber.deep || arePropsDifferent(currentProps, props);
     }
     if (shouldRender) {
@@ -148,6 +148,7 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
   status: STATUS = STATUS.NEW;
   forceNextRender: boolean = false;
   parentKey: string | null;
+  props: P;
 
   renderFn: Function;
   parent: ComponentNode | null;
@@ -174,11 +175,12 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
     currentNode = this;
     this.app = app;
     this.parent = parent;
+    this.props = props;
     this.parentKey = parentKey;
     this.level = parent ? parent.level + 1 : 0;
     const defaultProps = C.defaultProps;
     if (defaultProps) {
-      applyDefaultProps(props, defaultProps);
+      props = applyDefaultProps(props, defaultProps);
     }
     const env = (parent && parent.childEnv) || app.env;
     this.childEnv = env;
@@ -294,13 +296,14 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
   }
 
   async updateAndRender(props: P, parentFiber: Fiber) {
+    const rawProps = props;
     // update
     const fiber = makeChildFiber(this, parentFiber);
     this.fiber = fiber;
     const component = this.component;
     const defaultProps = (component.constructor as any).defaultProps;
     if (defaultProps) {
-      applyDefaultProps(props, defaultProps);
+      props = applyDefaultProps(props, defaultProps);
     }
 
     currentNode = this;
@@ -317,6 +320,7 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
       return;
     }
     component.props = props;
+    this.props = rawProps;
     fiber.render();
     const parentRoot = parentFiber.root!;
     if (this.willPatch.length) {
diff --git a/tests/components/__snapshots__/rendering.test.ts.snap b/tests/components/__snapshots__/rendering.test.ts.snap
index 8cd9a8b0..4ba191f1 100644
--- a/tests/components/__snapshots__/rendering.test.ts.snap
+++ b/tests/components/__snapshots__/rendering.test.ts.snap
@@ -1,5 +1,29 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`children, default props and renderings 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    const b2 = text(ctx['state'].value);
+    const b3 = component(\`Child\`, {}, key + \`__1\`, node, ctx);
+    return multi([b2, b3]);
+  }
+}"
+`;
+
+exports[`children, default props and renderings 2`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return text(\`child\`);
+  }
+}"
+`;
+
 exports[`force render in case of existing render 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
diff --git a/tests/components/rendering.test.ts b/tests/components/rendering.test.ts
index cb08437d..c9753473 100644
--- a/tests/components/rendering.test.ts
+++ b/tests/components/rendering.test.ts
@@ -477,3 +477,52 @@ test("force render in case of existing render", async () => {
     "A:patched",
   ]).toBeLogged();
 });
+
+test("children, default props and renderings", async () => {
+  class Child extends Component {
+    static template = xml`child`;
+    static defaultProps = { value: 1 };
+    setup() {
+      useLogLifecycle();
+    }
+  }
+
+  class Parent extends Component {
+    static template = xml`
+      <t t-esc="state.value"/>
+      <Child />
+    `;
+    static components = { Child };
+
+    state = useState({ value: "A" });
+    setup() {
+      useLogLifecycle();
+    }
+  }
+
+  const parent = await mount(Parent, fixture);
+
+  expect(fixture.innerHTML).toBe("Achild");
+  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 = "B";
+  await nextTick();
+  expect(fixture.innerHTML).toBe("Bchild");
+  expect([
+    "Parent:willRender",
+    "Parent:rendered",
+    "Parent:willPatch",
+    "Parent:patched",
+  ]).toBeLogged();
+});