From 4c77132ae2bb84bd1ccf8c98c7fb3a56a8163b93 Mon Sep 17 00:00:00 2001 From: Samuel Degueldre Date: Mon, 16 May 2022 15:00:20 +0200 Subject: [PATCH] [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 | 6 ++-- src/app/template_helpers.ts | 3 +- src/component/component_node.ts | 27 ++++++++++++---- .../__snapshots__/reactivity.test.ts.snap | 24 ++++++++++++++ tests/components/reactivity.test.ts | 32 +++++++++++++++++++ 5 files changed, 80 insertions(+), 12 deletions(-) diff --git a/doc/reference/reactivity.md b/doc/reference/reactivity.md index 48b24b45..75701a1d 100644 --- a/doc/reference/reactivity.md +++ b/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 -which part of these props are consumed by each component, and is therefore able -to only rerender the impacted components. +level: reactive objects received as props are automatically subscribed to by the +component, so Owl can track which part of these props are consumed by each +component, and is therefore able to only rerender the impacted components. ## `useState` diff --git a/src/app/template_helpers.ts b/src/app/template_helpers.ts index 4e35fcdb..65fe750d 100644 --- a/src/app/template_helpers.ts +++ b/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) { diff --git a/src/component/component_node.ts b/src/component/component_node.ts index b990d7e9..db613462 100644 --- a/src/component/component_node.ts +++ b/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

( +export function component

( name: string | ComponentConstructor

, props: P, key: string, @@ -92,7 +95,7 @@ export function component

( 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

( type LifecycleHook = Function; -export class ComponentNode

implements VNode> { +export class ComponentNode

implements VNode> { el?: HTMLElement | Text | undefined; app: App; fiber: Fiber | null = null; @@ -165,7 +168,12 @@ export class ComponentNode

implements VNode implements VNode implements VNode f.call(component, props))); await prom; diff --git a/tests/components/__snapshots__/reactivity.test.ts.snap b/tests/components/__snapshots__/reactivity.test.ts.snap index 29166b0c..8eacf43c 100644 --- a/tests/components/__snapshots__/reactivity.test.ts.snap +++ b/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 ) { diff --git a/tests/components/reactivity.test.ts b/tests/components/reactivity.test.ts index 997eef12..c336aa0a 100644 --- a/tests/components/reactivity.test.ts +++ b/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``; + setup() { + onWillRender(() => childRenderCount++); + } + } + class Parent extends Component { + static template = xml``; + 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"); + }); });