[IMP] component: only useState on props that are already reactive

Previously, components would automatically call useState on their props,
so that changes deeply within props would automatically cause the
component to be rendered. This can be useful when passing a piece of
state to children or descendants.

One problem with this is that all props implicitly become reactive, even
if the object that was passed as a props was not. The problem with that
being that since the original object is not reactive, any change made by
the parent will not go through the reactivity system and the children
won't be notified of the change, in essence, this reactive object is
essentially useless, while having a real cost: traversing reactive
objects creates more reactive objects, and those objects are all
proxies. This is expensive for basically no benefit, while also making
it more difficult to debug code that involves those objects.

This commit fixes that by only calling useState on objects that are
already reactive, allowing the usecase described in the first paragraph
without the drawbacks described in the second.

doc/reference/reactivity.md

@@ -26,9 +26,9 @@ To solve this issue, Owl provides two reactivity primitives:
 Most of the time, the `useState` hook is the best solution.
 
 Since version 2.0, Owl applies the fine grained reactivity at the component
-level: props are automatically turned into reactive object, so Owl can track
+level: reactive objects received as props are automatically subscribed to by the
-which part of these props are consumed by each component, and is therefore able
+component, so Owl can track which part of these props are consumed by each
-to only rerender the impacted components.
+component, and is therefore able to only rerender the impacted components.
 
 ## `useState`
 

src/app/template_helpers.ts

@@ -2,7 +2,6 @@ import { BDom, multi, text, toggler, createCatcher } from "../blockdom";
 import { validateProps } from "../component/props_validation";
 import { Markup } from "../utils";
 import { html } from "../blockdom/index";
-import { TARGET } from "../reactivity";
 
 /**
  * This file contains utility functions that will be injected in each template,
@@ -23,7 +22,7 @@ function callSlot(
   defaultContent?: (ctx: any, node: any, key: string) => BDom
 ): BDom {
   key = key + "__slot_" + name;
-  const slots = ctx.props[TARGET].slots || {};
+  const slots = ctx.props.slots || {};
   const { __render, __ctx, __scope } = slots[name] || {};
   const slotScope = Object.create(__ctx || {});
   if (__scope) {

src/component/component_node.ts

@@ -6,6 +6,7 @@ import {
   NonReactive,
   Reactive,
   reactive,
+  toRaw,
   TARGET,
 } from "../reactivity";
 import { batched, Callback } from "../utils";
@@ -62,14 +63,16 @@ type Props = { [key: string]: any };
 
 function arePropsDifferent(props1: Props, props2: Props): boolean {
   for (let k in props1) {
-    if (props1[k] !== props2[k]) {
+    const prop1 = props1[k] && typeof props1[k] === "object" ? toRaw(props1[k]) : props1[k];
+    const prop2 = props2[k] && typeof props2[k] === "object" ? toRaw(props2[k]) : props2[k];
+    if (prop1 !== prop2) {
       return true;
     }
   }
   return Object.keys(props1).length !== Object.keys(props2).length;
 }
 
-export function component<P extends object>(
+export function component<P extends Props>(
   name: string | ComponentConstructor<P>,
   props: P,
   key: string,
@@ -92,7 +95,7 @@ export function component<P extends object>(
     if (shouldRender) {
       node.forceNextRender = false;
     } else {
-      const currentProps = node.component.props[TARGET];
+      const currentProps = node.component.props;
       shouldRender = parentFiber.deep || arePropsDifferent(currentProps, props);
     }
     if (shouldRender) {
@@ -125,7 +128,7 @@ export function component<P extends object>(
 
 type LifecycleHook = Function;
 
-export class ComponentNode<P extends object = any, E = any> implements VNode<ComponentNode<P, E>> {
+export class ComponentNode<P extends Props = any, E = any> implements VNode<ComponentNode<P, E>> {
   el?: HTMLElement | Text | undefined;
   app: App;
   fiber: Fiber | null = null;
@@ -165,7 +168,12 @@ export class ComponentNode<P extends object = any, E = any> implements VNode<Com
     applyDefaultProps(props, C);
     const env = (parent && parent.childEnv) || app.env;
     this.childEnv = env;
-    props = useState(props);
+    for (const key in props) {
+      const prop = props[key];
+      if (prop && typeof prop === "object" && prop[TARGET]) {
+        props[key] = useState(prop);
+      }
+    }
     this.component = new C(props, env, this) as any;
     this.renderFn = app.getTemplate(C.template).bind(this.component, this.component, this);
     this.component.setup();
@@ -271,7 +279,7 @@ export class ComponentNode<P extends object = any, E = any> implements VNode<Com
     this.status = STATUS.DESTROYED;
   }
 
-  async updateAndRender(props: any, parentFiber: Fiber) {
+  async updateAndRender(props: P, parentFiber: Fiber) {
     // update
     const fiber = makeChildFiber(this, parentFiber);
     this.fiber = fiber;
@@ -279,7 +287,12 @@ export class ComponentNode<P extends object = any, E = any> implements VNode<Com
     applyDefaultProps(props, component.constructor as any);
 
     currentNode = this;
-    props = useState(props);
+    for (const key in props) {
+      const prop = props[key];
+      if (prop && typeof prop === "object" && prop[TARGET]) {
+        props[key] = useState(prop);
+      }
+    }
     currentNode = null;
     const prom = Promise.all(this.willUpdateProps.map((f) => f.call(component, props)));
     await prom;

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

@@ -26,6 +26,30 @@ exports[`reactivity in lifecycle Child component doesn't render when state they
 }"
 `;
 
+exports[`reactivity in lifecycle Component is automatically subscribed to reactive object received as prop 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return component(\`Child\`, {obj: ctx['obj'], reactiveObj: ctx['reactiveObj']}, key + \`__1\`, node, ctx);
+  }
+}"
+`;
+
+exports[`reactivity in lifecycle Component is automatically subscribed to reactive object received as prop 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    const b2 = text(ctx['props'].obj.a);
+    const b3 = text(ctx['props'].reactiveObj.b);
+    return multi([b2, b3]);
+  }
+}"
+`;
+
 exports[`reactivity in lifecycle can use a state hook 1`] = `
 "function anonymous(bdom, helpers
 ) {

tests/components/reactivity.test.ts

@@ -199,4 +199,36 @@ describe("reactivity in lifecycle", () => {
       "Parent:patched",
     ]).toBeLogged();
   });
+
+  test("Component is automatically subscribed to reactive object received as prop", async () => {
+    let childRenderCount = 0;
+    let parentRenderCount = 0;
+    class Child extends Component {
+      static template = xml`<t t-esc="props.obj.a"/><t t-esc="props.reactiveObj.b"/>`;
+      setup() {
+        onWillRender(() => childRenderCount++);
+      }
+    }
+    class Parent extends Component {
+      static template = xml`<Child obj="obj" reactiveObj="reactiveObj"/>`;
+      static components = { Child };
+      obj = { a: 1 };
+      reactiveObj = useState({ b: 2 });
+      setup() {
+        onWillRender(() => parentRenderCount++);
+      }
+    }
+    const comp = await mount(Parent, fixture);
+    expect([parentRenderCount, childRenderCount]).toEqual([1, 1]);
+    expect(fixture.innerHTML).toBe("12");
+    comp.obj.a = 3; // non reactive object, shouldn't cause render
+    await nextTick();
+    expect([parentRenderCount, childRenderCount]).toEqual([1, 1]);
+    expect(fixture.innerHTML).toBe("12");
+    comp.reactiveObj.b = 4;
+    await nextTick();
+    // Only child should be rendered: the parent never read the b key in reactiveObj
+    expect([parentRenderCount, childRenderCount]).toEqual([1, 2]);
+    expect(fixture.innerHTML).toBe("34");
+  });
 });