From a51b28667106f4f4cf3bbd01f7cf9ac3ae16d6e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Mon, 27 Feb 2023 07:57:33 +0100 Subject: [PATCH] [IMP] implement .alike suffix on props It is common in Owl components to have anonymous function as props. However, since each rendering create a different (but equivalent) closure, Owl will consider the props different, so will update the child component, but this is (often) not necessary. This commit will help reduce the problem by introducing a new `.alike` prop suffix, that will let Owl know that each version of that specific prop should be considered the same, so, will be ignored by the props comparison code. closes #1360 --- doc/reference/props.md | 45 ++++ src/compiler/code_generator.ts | 26 ++- src/runtime/app.ts | 37 ++- src/runtime/template_helpers.ts | 19 -- tests/__snapshots__/reactivity.test.ts.snap | 22 +- .../compiler/__snapshots__/misc.test.ts.snap | 6 +- .../__snapshots__/basics.test.ts.snap | 78 +++---- .../__snapshots__/concurrency.test.ts.snap | 168 +++++++------- .../__snapshots__/error_handling.test.ts.snap | 92 ++++---- .../__snapshots__/event_handling.test.ts.snap | 4 +- .../higher_order_component.test.ts.snap | 16 +- .../__snapshots__/hooks.test.ts.snap | 14 +- .../__snapshots__/lifecycle.test.ts.snap | 62 ++--- .../__snapshots__/props.test.ts.snap | 123 ++++++++-- .../props_validation.test.ts.snap | 148 ++++++------ .../__snapshots__/reactivity.test.ts.snap | 8 +- .../__snapshots__/refs.test.ts.snap | 4 +- .../__snapshots__/rendering.test.ts.snap | 24 +- .../__snapshots__/slots.test.ts.snap | 212 +++++++++--------- .../__snapshots__/style_class.test.ts.snap | 36 +-- .../__snapshots__/t_call.test.ts.snap | 28 +-- .../__snapshots__/t_component.test.ts.snap | 12 +- .../__snapshots__/t_foreach.test.ts.snap | 24 +- .../__snapshots__/t_key.test.ts.snap | 16 +- .../__snapshots__/t_on.test.ts.snap | 22 +- .../__snapshots__/t_out.test.ts.snap | 2 +- .../__snapshots__/t_props.test.ts.snap | 10 +- .../__snapshots__/t_set.test.ts.snap | 18 +- tests/components/props.test.ts | 163 +++++++++++++- tests/misc/__snapshots__/portal.test.ts.snap | 26 +-- 30 files changed, 879 insertions(+), 586 deletions(-) diff --git a/doc/reference/props.md b/doc/reference/props.md index 4fcfbd9b..7d673339 100644 --- a/doc/reference/props.md +++ b/doc/reference/props.md @@ -4,6 +4,7 @@ - [Overview](#overview) - [Definition](#definition) +- [Props comparison](#props-comparison) - [Binding function props](#binding-function-props) - [Dynamic Props](#dynamic-props) - [Default Props](#default-props) @@ -56,6 +57,47 @@ the `props` object contains the following keys: - for `ComponentA`: `a` and `b`, - for `ComponentB`: `model`, +## Props comparison + +Whenever Owl encounters a subcomponent in a template, it performs a shallow +comparison of all props. If they are all referentially equal, then the subcomponent +will not even be updated. Otherwise, if at least one props has changed, then +Owl will update it. + +However, in some cases, we know that two values are different, but they have the +same effect, and should not be considered different by Owl. For example, anonymous +functions in a template are always different, but most of them should not be +considered different: + +```xml + + + +``` + +In that case, one can use the `.alike` suffix: + +```xml + + + +``` + +This tells Owl that this specific prop should always be considered equivalent +(or, in other words, should be removed from the list of comparable props). + +Note that even if most anonymous functions should probably be considered `alike`, +it is not necessarily true in all cases. It depends on what values are captured +by the anonymous function. The following example shows a case where it is probably +wrong to use `.alike`. + +```xml + + + + +``` + ## Binding function props It is common to have the need to pass a callback as a prop. Since Owl components @@ -95,6 +137,9 @@ class SomeComponent extends Component { } ``` +The `.bind` suffix also implies `.alike`, so these props will not cause additional +renderings. + ## Dynamic Props The `t-props` directive can be used to specify totally dynamic props: diff --git a/src/compiler/code_generator.ts b/src/compiler/code_generator.ts index 626b8e26..18b99314 100644 --- a/src/compiler/code_generator.ts +++ b/src/compiler/code_generator.ts @@ -1132,12 +1132,15 @@ export class CodeGenerator { value = this.captureExpression(value); if (name.includes(".")) { let [_name, suffix] = name.split("."); - if (suffix === "bind") { - this.helpers.add("bind"); - name = _name; - value = `bind(this, ${value || undefined})`; - } else { - throw new OwlError("Invalid prop suffix"); + name = _name; + switch (suffix) { + case "bind": + value = `${value}.bind(this)`; + break; + case "alike": + break; + default: + throw new OwlError("Invalid prop suffix"); } } name = /^[a-z_]+$/i.test(name) ? name : `'${name}'`; @@ -1237,13 +1240,18 @@ export class CodeGenerator { keyArg = `${ctx.tKeyExpr} + ${keyArg}`; } let id = generateId("comp"); + const propList: string[] = []; + for (let p in ast.props || {}) { + let [name, suffix] = p.split("."); + if (!suffix) { + propList.push(`"${name}"`); + } + } this.staticDefs.push({ id, expr: `app.createComponent(${ ast.isDynamic ? null : expr - }, ${!ast.isDynamic}, ${!!ast.slots}, ${!!ast.dynamicProps}, ${ - !ast.props && !ast.dynamicProps - })`, + }, ${!ast.isDynamic}, ${!!ast.slots}, ${!!ast.dynamicProps}, [${propList}])`, }); if (ast.isDynamic) { diff --git a/src/runtime/app.ts b/src/runtime/app.ts index 1b4b1946..caca43f5 100644 --- a/src/runtime/app.ts +++ b/src/runtime/app.ts @@ -139,22 +139,35 @@ export class App< isStatic: boolean, hasSlotsProp: boolean, hasDynamicPropList: boolean, - hasNoProp: boolean + propList: string[] ) { const isDynamic = !isStatic; - function _arePropsDifferent(props1: Props, props2: Props): boolean { - for (let k in props1) { - if (props1[k] !== props2[k]) { - return true; + let arePropsDifferent: (p1: Object, p2: Object) => boolean; + const hasNoProp = propList.length === 0; + if (hasSlotsProp) { + arePropsDifferent = (_1, _2) => true; + } else if (hasDynamicPropList) { + arePropsDifferent = function (props1: Props, props2: Props) { + for (let k in props1) { + if (props1[k] !== props2[k]) { + return true; + } } - } - return hasDynamicPropList && Object.keys(props1).length !== Object.keys(props2).length; + return Object.keys(props1).length !== Object.keys(props2).length; + }; + } else if (hasNoProp) { + arePropsDifferent = (_1: any, _2: any) => false; + } else { + arePropsDifferent = function (props1: Props, props2: Props) { + for (let p of propList) { + if (props1[p] !== props2[p]) { + return true; + } + } + return false; + }; } - const arePropsDifferent = hasSlotsProp - ? (_1: any, _2: any) => true - : hasNoProp - ? (_1: any, _2: any) => false - : _arePropsDifferent; + const updateAndRender = ComponentNode.prototype.updateAndRender; const initiateRender = ComponentNode.prototype.initiateRender; diff --git a/src/runtime/template_helpers.ts b/src/runtime/template_helpers.ts index f472a6d5..e786f223 100644 --- a/src/runtime/template_helpers.ts +++ b/src/runtime/template_helpers.ts @@ -167,24 +167,6 @@ export function safeOutput(value: any, defaultValue?: any): ReturnType\`); @@ -82,7 +82,7 @@ exports[`Reactivity: useState destroyed component is inactive 1`] = ` "function anonymous(app, bdom, helpers ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - const comp1 = app.createComponent(\`Child\`, true, false, false, true); + const comp1 = app.createComponent(\`Child\`, true, false, false, []); let block1 = createBlock(\`
\`); @@ -129,7 +129,7 @@ exports[`Reactivity: useState parent and children subscribed to same context 1`] "function anonymous(app, bdom, helpers ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - const comp1 = app.createComponent(\`Child\`, true, false, false, true); + const comp1 = app.createComponent(\`Child\`, true, false, false, []); let block1 = createBlock(\`
\`); @@ -222,8 +222,8 @@ exports[`Reactivity: useState two components are updated in parallel 1`] = ` "function anonymous(app, bdom, helpers ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - const comp1 = app.createComponent(\`Child\`, true, false, false, true); - const comp2 = app.createComponent(\`Child\`, true, false, false, true); + const comp1 = app.createComponent(\`Child\`, true, false, false, []); + const comp2 = app.createComponent(\`Child\`, true, false, false, []); let block1 = createBlock(\`
\`); @@ -253,8 +253,8 @@ exports[`Reactivity: useState two components can subscribe to same context 1`] = "function anonymous(app, bdom, helpers ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - const comp1 = app.createComponent(\`Child\`, true, false, false, true); - const comp2 = app.createComponent(\`Child\`, true, false, false, true); + const comp1 = app.createComponent(\`Child\`, true, false, false, []); + const comp2 = app.createComponent(\`Child\`, true, false, false, []); let block1 = createBlock(\`
\`); @@ -284,8 +284,8 @@ exports[`Reactivity: useState two independent components on different levels are "function anonymous(app, bdom, helpers ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - const comp1 = app.createComponent(\`Child\`, true, false, false, true); - const comp2 = app.createComponent(\`Parent\`, true, false, false, true); + const comp1 = app.createComponent(\`Child\`, true, false, false, []); + const comp2 = app.createComponent(\`Parent\`, true, false, false, []); let block1 = createBlock(\`
\`); @@ -315,7 +315,7 @@ exports[`Reactivity: useState two independent components on different levels are "function anonymous(app, bdom, helpers ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - const comp1 = app.createComponent(\`Child\`, true, false, false, true); + const comp1 = app.createComponent(\`Child\`, true, false, false, []); let block1 = createBlock(\`
\`); @@ -345,7 +345,7 @@ exports[`Reactivity: useState useless atoms should be deleted 1`] = ` ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; let { prepareList, withKey } = helpers; - const comp1 = app.createComponent(\`Quantity\`, true, false, false, false); + const comp1 = app.createComponent(\`Quantity\`, true, false, false, [\\"id\\"]); let block1 = createBlock(\`
Total: Count:
\`); diff --git a/tests/compiler/__snapshots__/misc.test.ts.snap b/tests/compiler/__snapshots__/misc.test.ts.snap index 2d3e8f81..2d4aa3bf 100644 --- a/tests/compiler/__snapshots__/misc.test.ts.snap +++ b/tests/compiler/__snapshots__/misc.test.ts.snap @@ -5,7 +5,7 @@ exports[`misc complex template 1`] = ` ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; let { prepareList, withKey } = helpers; - const comp1 = app.createComponent(\`SlotButton\`, true, false, false, false); + const comp1 = app.createComponent(\`SlotButton\`, true, false, false, [\\"class\\",\\"slot\\"]); let block1 = createBlock(\`
\`); let block2 = createBlock(\`\`); @@ -184,8 +184,8 @@ exports[`misc other complex template 1`] = ` let { text, createBlock, list, multi, html, toggler, comment } = bdom; let { prepareList, withKey } = helpers; const callTemplate_1 = app.getTemplate(\`LOAD_INFOS_TEMPLATE\`); - const comp1 = app.createComponent(\`BundlesList\`, true, false, false, false); - const comp2 = app.createComponent(\`BundlesList\`, true, false, false, false); + const comp1 = app.createComponent(\`BundlesList\`, true, false, false, [\\"bundles\\",\\"category_custom_views\\",\\"search\\"]); + const comp2 = app.createComponent(\`BundlesList\`, true, false, false, [\\"bundles\\",\\"search\\"]); let block1 = createBlock(\`