From f892929c802b0e64484032c5dc9de8bfb45677f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Wed, 1 Mar 2023 11:50:30 +0100 Subject: [PATCH] [REF] blockdom,compiler: implement properties Before this commit, properties were just handled as a special case of attributes. But it does not make that much sense, since they are different. For example, the `readOnly` property does not have the same name as the `readonly` attribute. The confusion probably comes from the fact that QWeb does not distinguish between property and attributes, so Owl has to infer which one is which. With this commit, we introduce a `block-property` directive in blockdom, and change the compiler to use it in emitted code. It has the additional benefits of slightly shrinking the runtime code, since the `isProp` function can now be located in the compiler. --- src/compiler/code_generator.ts | 45 +++-- src/runtime/blockdom/attributes.ts | 30 ---- src/runtime/blockdom/block_compiler.ts | 44 +++-- tests/blockdom/block_attributes.test.ts | 1 - tests/blockdom/block_properties.test.ts | 12 +- .../compiler/__snapshots__/misc.test.ts.snap | 26 +-- .../__snapshots__/properties.test.ts.snap | 72 ++++---- tests/compiler/properties.test.ts | 1 - .../__snapshots__/t_model.test.ts.snap | 168 +++++++++--------- 9 files changed, 203 insertions(+), 196 deletions(-) diff --git a/src/compiler/code_generator.ts b/src/compiler/code_generator.ts index 005700e1..f3b74f94 100644 --- a/src/compiler/code_generator.ts +++ b/src/compiler/code_generator.ts @@ -1,4 +1,3 @@ -import { isProp } from "../runtime/blockdom/attributes"; import { compileExpr, compileExprToArray, @@ -58,6 +57,29 @@ function generateId(prefix: string = "") { return prefix + nextDataIds[prefix]; } +function isProp(tag: string, key: string): boolean { + switch (tag) { + case "input": + return ( + key === "checked" || + key === "indeterminate" || + key === "value" || + key === "readonly" || + key === "disabled" + ); + case "option": + return key === "selected" || key === "disabled"; + case "textarea": + return key === "value" || key === "readonly" || key === "disabled"; + case "select": + return key === "value" || key === "disabled"; + case "button": + case "optgroup": + return key === "disabled"; + } + return false; +} + // ----------------------------------------------------------------------------- // BlockDescription // ----------------------------------------------------------------------------- @@ -585,12 +607,15 @@ export class CodeGenerator { } else { expr = `new Boolean(${expr})`; } - } - const idx = block!.insertData(expr, "attr"); - if (key === "t-att") { - attrs[`block-attributes`] = String(idx); + const idx = block!.insertData(expr, "prop"); + attrs[`block-property-${idx}`] = attrName!; } else { - attrs[`block-attribute-${idx}`] = attrName!; + const idx = block!.insertData(expr, "attr"); + if (key === "t-att") { + attrs[`block-attributes`] = String(idx); + } else { + attrs[`block-attribute-${idx}`] = attrName!; + } } } else if (this.translatableAttributes.includes(key)) { attrs[key] = this.translateFn(ast.attrs[key]); @@ -640,15 +665,15 @@ export class CodeGenerator { targetExpr = compileExpr(dynamicTgExpr); } } - idx = block!.insertData(`${fullExpression} === ${targetExpr}`, "attr"); - attrs[`block-attribute-${idx}`] = specialInitTargetAttr; + idx = block!.insertData(`${fullExpression} === ${targetExpr}`, "prop"); + attrs[`block-property-${idx}`] = specialInitTargetAttr; } else if (hasDynamicChildren) { const bValueId = generateId("bValue"); tModelSelectedExpr = `${bValueId}`; this.define(tModelSelectedExpr, fullExpression); } else { - idx = block!.insertData(`${fullExpression}`, "attr"); - attrs[`block-attribute-${idx}`] = targetAttr; + idx = block!.insertData(`${fullExpression}`, "prop"); + attrs[`block-property-${idx}`] = targetAttr; } this.helpers.add("toNumber"); let valueCode = `ev.target.${targetAttr}`; diff --git a/src/runtime/blockdom/attributes.ts b/src/runtime/blockdom/attributes.ts index 32a9472e..422ed76a 100644 --- a/src/runtime/blockdom/attributes.ts +++ b/src/runtime/blockdom/attributes.ts @@ -140,33 +140,3 @@ export function updateClass(this: HTMLElement, val: any, oldVal: any) { } } } - -export function makePropSetter(name: string): Setter { - return function setProp(this: HTMLElement, value: any) { - // support 0, fallback to empty string for other falsy values - (this as any)[name] = value === 0 ? 0 : value ? value.valueOf() : ""; - }; -} - -export function isProp(tag: string, key: string): boolean { - switch (tag) { - case "input": - return ( - key === "checked" || - key === "indeterminate" || - key === "value" || - key === "readonly" || - key === "disabled" - ); - case "option": - return key === "selected" || key === "disabled"; - case "textarea": - return key === "value" || key === "readonly" || key === "disabled"; - case "select": - return key === "value" || key === "disabled"; - case "button": - case "optgroup": - return key === "disabled"; - } - return false; -} diff --git a/src/runtime/blockdom/block_compiler.ts b/src/runtime/blockdom/block_compiler.ts index 9857bdfa..e67d7132 100644 --- a/src/runtime/blockdom/block_compiler.ts +++ b/src/runtime/blockdom/block_compiler.ts @@ -1,13 +1,5 @@ import { OwlError } from "../error_handling"; -import { - attrsSetter, - attrsUpdater, - createAttrUpdater, - isProp, - makePropSetter, - setClass, - updateClass, -} from "./attributes"; +import { attrsSetter, attrsUpdater, createAttrUpdater, setClass, updateClass } from "./attributes"; import { config } from "./config"; import { createEventHandler } from "./events"; import type { VNode } from "./index"; @@ -25,6 +17,13 @@ const nodeGetNextSibling = getDescriptor(nodeProto, "nextSibling").get!; const NO_OP = () => {}; +function makePropSetter(name: string): Setter { + return function setProp(this: HTMLElement, value: any) { + // support 0, fallback to empty string for other falsy values + (this as any)[name] = value === 0 ? 0 : value ? value.valueOf() : ""; + }; +} + // ----------------------------------------------------------------------------- // Main compiler code // ----------------------------------------------------------------------------- @@ -101,7 +100,7 @@ function normalizeNode(node: HTMLElement | Text) { interface DynamicInfo { idx: number; refIdx?: number; - type: "text" | "child" | "handler" | "attribute" | "attributes" | "ref"; + type: "text" | "child" | "handler" | "attribute" | "attributes" | "property" | "ref"; isOnlyChild?: boolean; name?: string; tag?: string; @@ -184,6 +183,14 @@ function buildTree( name: attrValue, tag: tagName, }); + } else if (attrName.startsWith("block-property-")) { + const idx = parseInt(attrName.slice(15), 10); + info.push({ + type: "property", + idx, + name: attrValue, + tag: tagName, + }); } else if (attrName === "block-attributes") { info.push({ type: "attributes", @@ -377,15 +384,22 @@ function updateCtx(ctx: BlockCtx, tree: IntermediateTree) { }; } break; + case "property": { + const refIdx = info.refIdx!; + const setProp = makePropSetter(info.name!); + ctx.locations.push({ + idx: info.idx, + refIdx, + setData: setProp, + updateData: setProp, + }); + break; + } case "attribute": { const refIdx = info.refIdx!; let updater: any; let setter: any; - if (isProp(info.tag!, info.name!)) { - const setProp = makePropSetter(info.name!); - setter = setProp; - updater = setProp; - } else if (info.name === "class") { + if (info.name === "class") { setter = setClass; updater = updateClass; } else { diff --git a/tests/blockdom/block_attributes.test.ts b/tests/blockdom/block_attributes.test.ts index 40aa66f1..1085473b 100644 --- a/tests/blockdom/block_attributes.test.ts +++ b/tests/blockdom/block_attributes.test.ts @@ -145,4 +145,3 @@ test("class attribute (with a preexisting value", async () => { patch(tree, block([""])); expect(fixture.innerHTML).toBe(`
`); }); - diff --git a/tests/blockdom/block_properties.test.ts b/tests/blockdom/block_properties.test.ts index cd47b534..5fa75b82 100644 --- a/tests/blockdom/block_properties.test.ts +++ b/tests/blockdom/block_properties.test.ts @@ -15,9 +15,9 @@ afterEach(() => { fixture.remove(); }); -test("input with value attribute", () => { +test("input with value property", () => { // render input with initial value - const block = createBlock(``); + const block = createBlock(``); const tree = block(["zucchini"]); mount(tree, fixture); @@ -37,8 +37,8 @@ test("input with value attribute", () => { expect(input.value).toBe("potato"); }); -test("input with value attribute, and falsy value given", () => { - const block = createBlock(``); +test("input with value property, and falsy value given", () => { + const block = createBlock(``); const tree = block([undefined]); mount(tree, fixture); @@ -58,9 +58,9 @@ test("input with value attribute, and falsy value given", () => { expect(input.value).toBe(""); }); -test("input type=checkbox with checked attribute", () => { +test("input type=checkbox with checked property", () => { // render input with initial value - const block = createBlock(``); + const block = createBlock(``); const tree = block([true]); mount(tree, fixture); diff --git a/tests/compiler/__snapshots__/misc.test.ts.snap b/tests/compiler/__snapshots__/misc.test.ts.snap index d980ad93..fa3a47a3 100644 --- a/tests/compiler/__snapshots__/misc.test.ts.snap +++ b/tests/compiler/__snapshots__/misc.test.ts.snap @@ -187,7 +187,7 @@ exports[`misc other complex template 1`] = ` const comp1 = app.createComponent(\`BundlesList\`, true, false, false, false); const comp2 = app.createComponent(\`BundlesList\`, true, false, false, false); - let block1 = createBlock(\`
\`); let block24 = createBlock(\`

No project

\`); @@ -269,15 +269,15 @@ exports[`misc other complex template 1`] = ` ctx[\`category\`] = v_block15[i1]; const key1 = ctx['category'].id; let attr6 = ctx['category'].id; - let attr7 = new Boolean(ctx['category'].id==ctx['options'].active_category_id); + let prop1 = new Boolean(ctx['category'].id==ctx['options'].active_category_id); let txt5 = ctx['category'].name; - c_block15[i1] = withKey(block16([attr6, attr7, txt5]), key1); + c_block15[i1] = withKey(block16([attr6, prop1, txt5]), key1); } ctx = ctx.__proto__; const b15 = list(c_block15); b14 = block14([], [b15]); } - let attr8 = new String((ctx['search'].value) === 0 ? 0 : ((ctx['search'].value) || \\"\\")); + let prop2 = new String((ctx['search'].value) === 0 ? 0 : ((ctx['search'].value) || \\"\\")); let hdlr4 = [ctx['updateFilter'], ctx]; let hdlr5 = [ctx['updateFilter'], ctx]; let hdlr6 = [ctx['clearSearch'], ctx]; @@ -289,14 +289,14 @@ exports[`misc other complex template 1`] = ` const key1 = ctx['trigger'].id; let b20; if (!ctx['trigger'].manual&&ctx['trigger'].project_id===ctx['project'].id&&ctx['trigger'].category_id===ctx['options'].active_category_id) { - let attr9 = \`trigger_\${ctx['trigger'].id}\`; - let attr10 = \`trigger_\${ctx['trigger'].id}\`; - let attr11 = new Boolean(ctx['options'].trigger_display[ctx['trigger'].id]); - let attr12 = ctx['trigger'].id; + let attr7 = \`trigger_\${ctx['trigger'].id}\`; + let attr8 = \`trigger_\${ctx['trigger'].id}\`; + let prop3 = new Boolean(ctx['options'].trigger_display[ctx['trigger'].id]); + let attr9 = ctx['trigger'].id; let hdlr7 = [ctx['updateTriggerDisplay'], ctx]; - let attr13 = \`trigger_\${ctx['trigger'].id}\`; + let attr10 = \`trigger_\${ctx['trigger'].id}\`; let txt6 = ctx['trigger'].name; - b20 = block20([attr9, attr10, attr11, attr12, hdlr7, attr13, txt6]); + b20 = block20([attr7, attr8, prop3, attr9, hdlr7, attr10, txt6]); } c_block18[i1] = withKey(multi([b20]), key1); } @@ -323,7 +323,7 @@ exports[`misc other complex template 1`] = ` const b27 = comp2({bundles: ctx['bundles'].dev,search: ctx['search']}, key + \`__3\`, node, this, null); b25 = block25([], [b26, b27]); } - return block1([attr1, txt1, hdlr2, hdlr3, attr8, hdlr4, hdlr5, ref1, hdlr6, ref2], [b2, b4, b14, b17, b22, b23, b24, b25]); + return block1([attr1, txt1, hdlr2, hdlr3, prop2, hdlr4, hdlr5, ref1, hdlr6, ref2], [b2, b4, b14, b17, b22, b23, b24, b25]); } }" `; diff --git a/tests/compiler/__snapshots__/properties.test.ts.snap b/tests/compiler/__snapshots__/properties.test.ts.snap index 3c115bc5..4abcc25a 100644 --- a/tests/compiler/__snapshots__/properties.test.ts.snap +++ b/tests/compiler/__snapshots__/properties.test.ts.snap @@ -19,11 +19,11 @@ exports[`dynamic input value: falsy values 1`] = ` ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - let block1 = createBlock(\`\`); + let block1 = createBlock(\`\`); return function template(ctx, node, key = \\"\\") { - let attr1 = new String((0) === 0 ? 0 : ((0) || \\"\\")); - return block1([attr1]); + let prop1 = new String((0) === 0 ? 0 : ((0) || \\"\\")); + return block1([prop1]); } }" `; @@ -33,11 +33,11 @@ exports[`dynamic input value: falsy values 2`] = ` ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - let block1 = createBlock(\`\`); + let block1 = createBlock(\`\`); return function template(ctx, node, key = \\"\\") { - let attr1 = new String((false) === 0 ? 0 : ((false) || \\"\\")); - return block1([attr1]); + let prop1 = new String((false) === 0 ? 0 : ((false) || \\"\\")); + return block1([prop1]); } }" `; @@ -47,11 +47,11 @@ exports[`dynamic input value: falsy values 3`] = ` ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - let block1 = createBlock(\`\`); + let block1 = createBlock(\`\`); return function template(ctx, node, key = \\"\\") { - let attr1 = new String((undefined) === 0 ? 0 : ((undefined) || \\"\\")); - return block1([attr1]); + let prop1 = new String((undefined) === 0 ? 0 : ((undefined) || \\"\\")); + return block1([prop1]); } }" `; @@ -61,11 +61,11 @@ exports[`dynamic input value: falsy values 4`] = ` ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - let block1 = createBlock(\`\`); + let block1 = createBlock(\`\`); return function template(ctx, node, key = \\"\\") { - let attr1 = new String(('') === 0 ? 0 : (('') || \\"\\")); - return block1([attr1]); + let prop1 = new String(('') === 0 ? 0 : (('') || \\"\\")); + return block1([prop1]); } }" `; @@ -75,11 +75,11 @@ exports[`input of type checkbox with t-att-indeterminate 1`] = ` ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - let block1 = createBlock(\`\`); + let block1 = createBlock(\`\`); return function template(ctx, node, key = \\"\\") { - let attr1 = new Boolean(ctx['v']); - return block1([attr1]); + let prop1 = new Boolean(ctx['v']); + return block1([prop1]); } }" `; @@ -89,11 +89,11 @@ exports[`input type= checkbox, with t-att-checked 1`] = ` ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - let block1 = createBlock(\`\`); + let block1 = createBlock(\`\`); return function template(ctx, node, key = \\"\\") { - let attr1 = new Boolean(ctx['flag']); - return block1([attr1]); + let prop1 = new Boolean(ctx['flag']); + return block1([prop1]); } }" `; @@ -103,11 +103,11 @@ exports[`input with t-att-value (patching with same value 1`] = ` ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - let block1 = createBlock(\`\`); + let block1 = createBlock(\`\`); return function template(ctx, node, key = \\"\\") { - let attr1 = new String((ctx['v']) === 0 ? 0 : ((ctx['v']) || \\"\\")); - return block1([attr1]); + let prop1 = new String((ctx['v']) === 0 ? 0 : ((ctx['v']) || \\"\\")); + return block1([prop1]); } }" `; @@ -117,11 +117,11 @@ exports[`input with t-att-value 1`] = ` ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - let block1 = createBlock(\`\`); + let block1 = createBlock(\`\`); return function template(ctx, node, key = \\"\\") { - let attr1 = new String((ctx['v']) === 0 ? 0 : ((ctx['v']) || \\"\\")); - return block1([attr1]); + let prop1 = new String((ctx['v']) === 0 ? 0 : ((ctx['v']) || \\"\\")); + return block1([prop1]); } }" `; @@ -131,11 +131,11 @@ exports[`input, type checkbox, with t-att-checked (patching with same value 1`] ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - let block1 = createBlock(\`\`); + let block1 = createBlock(\`\`); return function template(ctx, node, key = \\"\\") { - let attr1 = new Boolean(ctx['v']); - return block1([attr1]); + let prop1 = new Boolean(ctx['v']); + return block1([prop1]); } }" `; @@ -145,11 +145,11 @@ exports[`select with t-att-value 1`] = ` ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - let block1 = createBlock(\`\`); + let block1 = createBlock(\`\`); return function template(ctx, node, key = \\"\\") { - let attr1 = new String((ctx['value']) === 0 ? 0 : ((ctx['value']) || \\"\\")); - return block1([attr1]); + let prop1 = new String((ctx['value']) === 0 ? 0 : ((ctx['value']) || \\"\\")); + return block1([prop1]); } }" `; @@ -159,11 +159,11 @@ exports[`textarea with t-att-value 1`] = ` ) { let { text, createBlock, list, multi, html, toggler, comment } = bdom; - let block1 = createBlock(\`