From a122a941802e8344c9c28590097d67bd5442e7c5 Mon Sep 17 00:00:00 2001 From: Samuel Degueldre Date: Wed, 10 Nov 2021 08:18:25 +0100 Subject: [PATCH] [IMP] qweb/components: remove t-ref on components Refs to component expose a lot of implementation details that should be private to parents. Parent to child communication should go through props. --- src/qweb/compiler.ts | 6 +- src/qweb/parser.ts | 7 +- src/refs.ts | 24 +--- .../__snapshots__/refs.test.ts.snap | 16 +++ .../__snapshots__/style_class.test.ts.snap | 27 +++++ tests/components/hooks.test.ts | 2 +- tests/components/refs.test.ts | 109 ++---------------- tests/components/style_class.test.ts | 24 ++-- 8 files changed, 73 insertions(+), 142 deletions(-) diff --git a/src/qweb/compiler.ts b/src/qweb/compiler.ts index c76e4196..7b080daa 100644 --- a/src/qweb/compiler.ts +++ b/src/qweb/compiler.ts @@ -261,8 +261,7 @@ export class QWebCompiler { return block; } - insertBlock(expression: string, block: BlockDescription, ctx: Context): string | null { - let id: string | null = null; + insertBlock(expression: string, block: BlockDescription, ctx: Context): void { let blockExpr = block.generateExpr(expression); const tKeyExpr = ctx.tKeyExpr; if (block.parentVar) { @@ -271,7 +270,7 @@ export class QWebCompiler { keyArg = `${tKeyExpr} + ${keyArg}`; } this.addLine(`${block.parentVar}[${ctx.index}] = withKey(${blockExpr}, ${keyArg});`); - return id; + return; } if (tKeyExpr) { @@ -283,7 +282,6 @@ export class QWebCompiler { } else { this.addLine(`let ${block.varName} = ${blockExpr};`); } - return id; } generateCode(): string { diff --git a/src/qweb/parser.ts b/src/qweb/parser.ts index e4401492..0ead2822 100644 --- a/src/qweb/parser.ts +++ b/src/qweb/parser.ts @@ -302,11 +302,8 @@ function parseDOMNode(node: Element, ctx: ParsingContext): AST | null { if (tagName === "pre") { ctx = { inPreTag: true }; } - let ref = null; - if (node.hasAttribute("t-ref")) { - ref = node.getAttribute("t-ref"); - node.removeAttribute("t-ref"); - } + const ref = node.getAttribute("t-ref"); + node.removeAttribute("t-ref"); for (let child of node.childNodes) { const ast = parseNode(child, ctx); diff --git a/src/refs.ts b/src/refs.ts index 705d47b9..ac1fc44f 100644 --- a/src/refs.ts +++ b/src/refs.ts @@ -2,35 +2,17 @@ // useRef // ----------------------------------------------------------------------------- -import type { Component } from "./component/component"; import { getCurrent } from "./component/component_node"; /** * The purpose of this hook is to allow components to get a reference to a sub * html node or component. */ -interface Ref { - el: HTMLElement | null; - comp: C | null; -} - -export function useRef(name: string): Ref { +export function useRef(name: string): { el: T | null } { const node = getCurrent()!; return { - get el(): HTMLElement | null { - const val = node.refs[name]; - return val || null; - // if (val instanceof HTMLElement) { - // return val; - // } else if (val instanceof Component) { - // return val.el; - // } - // return null; - }, - get comp(): C | null { - return null; - // const val = node.refs && node.refs[name]; - // return val instanceof Component ? (val as C) : null; + get el(): T | null { + return node.refs[name] || null; }, }; } diff --git a/tests/components/__snapshots__/refs.test.ts.snap b/tests/components/__snapshots__/refs.test.ts.snap index 1d4e5c1d..e2caae75 100644 --- a/tests/components/__snapshots__/refs.test.ts.snap +++ b/tests/components/__snapshots__/refs.test.ts.snap @@ -1,5 +1,21 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`refs basic use 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component } = bdom; + let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber } = helpers; + + let block1 = createBlock(\`
\`); + + return function template(ctx, node, key = \\"\\") { + const refs = ctx.__owl__.refs; + let d1 = (el) => refs[\`div\`] = el; + return block1([d1]); + } +}" +`; + exports[`refs refs are properly bound in slots 1`] = ` "function anonymous(bdom, helpers ) { diff --git a/tests/components/__snapshots__/style_class.test.ts.snap b/tests/components/__snapshots__/style_class.test.ts.snap index 26672ca4..1a638694 100644 --- a/tests/components/__snapshots__/style_class.test.ts.snap +++ b/tests/components/__snapshots__/style_class.test.ts.snap @@ -490,3 +490,30 @@ exports[`style and class handling t-att-class is properly added/removed on widge } }" `; + +exports[`style and class handling t-att-class is properly added/removed on widget root el (v3) 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component } = bdom; + let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber } = helpers; + + let block1 = createBlock(\`\`); + + return function template(ctx, node, key = \\"\\") { + let d1 = {d:ctx['state'].d,...ctx['props'].class}; + return block1([d1]); + } +}" +`; + +exports[`style and class handling t-att-class is properly added/removed on widget root el (v3) 2`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component } = bdom; + let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber } = helpers; + + return function template(ctx, node, key = \\"\\") { + return component(\`Child\`, {class: {a:true,b:ctx['state'].b}}, key + \`__1\`, node, ctx); + } +}" +`; diff --git a/tests/components/hooks.test.ts b/tests/components/hooks.test.ts index e767feae..09da82e6 100644 --- a/tests/components/hooks.test.ts +++ b/tests/components/hooks.test.ts @@ -34,7 +34,7 @@ describe("hooks", () => { } increment() { this.value++; - (this.button.el as HTMLButtonElement).innerHTML = String(this.value); + this.button.el!.innerHTML = String(this.value); } } const mounted = mount(Counter, fixture); diff --git a/tests/components/refs.test.ts b/tests/components/refs.test.ts index 03fa8506..43b15f38 100644 --- a/tests/components/refs.test.ts +++ b/tests/components/refs.test.ts @@ -10,6 +10,15 @@ beforeEach(() => { }); describe("refs", () => { + test("basic use", async () => { + class Test extends Component { + static template = xml`
`; + button = useRef("div"); + } + const test = await mount(Test, fixture); + expect(test.button.el).toBe(fixture.firstChild); + }); + test("refs are properly bound in slots", async () => { class Dialog extends Component { static template = xml``; @@ -44,104 +53,4 @@ describe("refs", () => { '
1
' ); }); - // TODO: rename - test.skip("t-refs on widget are components", async () => { - class Child extends Component { - static template = xml`
b
`; - } - let parent: Parent; - class Parent extends Component { - static template = xml`
Hello
`; - static components = { Child }; - ref = useRef("mywidgetb"); - setup() { - parent = this; - } - } - - const mounted = mount(Parent, fixture); - expect(parent!.ref.comp).toBe(null); - expect(parent!.ref.el).toBe(null); - await mounted; - expect(parent!.ref.comp).toBeInstanceOf(Child); - expect(parent!.ref.el).toEqual(fixture.querySelector(".outer-div > div")); - }); - - test.skip("t-refs are bound at proper timing", async () => { - expect.assertions(4); - class Child extends Component { - static template = xml`
widget
`; - } - - class Parent extends Component { - static template = xml` -
- -
- `; - static components = { Child }; - state = useState({ list: [] }); - child = useRef("child"); - willPatch() { - expect(this.child.comp).toBeNull(); - } - patched() { - expect(this.child.comp).not.toBeNull(); - } - } - - const parent = await mount(Parent, fixture); - parent.state.list.push(1); - await nextTick(); - }); - - test.skip("t-refs are bound at proper timing (2)", async () => { - expect.assertions(10); - class Child extends Component { - static template = xml`
widget
`; - } - class Parent extends Component { - static template = xml` -
- - -
`; - static components = { Child }; - state = useState({ child1: true, child2: false }); - child1 = useRef("child1"); - child2 = useRef("child2"); - count = 0; - mounted() { - expect(this.child1.comp).toBeDefined(); - expect(this.child2.comp).toBeNull(); - } - willPatch() { - if (this.count === 0) { - expect(this.child1.comp).toBeDefined(); - expect(this.child2.comp).toBeNull(); - } - if (this.count === 1) { - expect(this.child1.comp).toBeDefined(); - expect(this.child2.comp).toBeDefined(); - } - } - patched() { - if (this.count === 0) { - expect(this.child1.comp).toBeDefined(); - expect(this.child2.comp).toBeDefined(); - } - if (this.count === 1) { - expect(this.child1.comp).toBeNull(); - expect(this.child2.comp).toBeDefined(); - } - this.count++; - } - } - - const parent = await mount(Parent, fixture); - parent.state.child2 = true; - await nextTick(); - parent.state.child1 = false; - await nextTick(); - }); }); diff --git a/tests/components/style_class.test.ts b/tests/components/style_class.test.ts index 2f75798f..378fc599 100644 --- a/tests/components/style_class.test.ts +++ b/tests/components/style_class.test.ts @@ -1,4 +1,4 @@ -import { Component, mount, useState, useRef, xml } from "../../src"; +import { Component, mount, useState, xml } from "../../src"; import { makeTestFixture, nextTick, snapshotEverything } from "../helpers"; snapshotEverything(); @@ -197,7 +197,7 @@ describe("style and class handling", () => { // TODO: adapt name (t-att-class no longer makes sense on Components) test("t-att-class is properly added/removed on widget root el (v2)", async () => { - let child: any = null; + let child: Child; class Child extends Component { static template = xml``; state = useState({ d: true }); @@ -223,7 +223,7 @@ describe("style and class handling", () => { await nextTick(); expect(span.className).toBe("c d"); - child.state.d = false; + child!.state.d = false; await nextTick(); expect(span.className).toBe("c"); @@ -231,23 +231,25 @@ describe("style and class handling", () => { await nextTick(); expect(span.className).toBe("c b"); - child.state.d = true; + child!.state.d = true; await nextTick(); expect(span.className).toBe("c b d"); }); // TODO: adapt name (t-att-class no longer makes sense on Components) - // TODO: this test depends on ref to components - test.skip("t-att-class is properly added/removed on widget root el (v3)", async () => { + test("t-att-class is properly added/removed on widget root el (v3)", async () => { + let child: Child; class Child extends Component { - static template = ``; + static template = xml``; state = useState({ d: true }); + setup() { + child = this; + } } class Parent extends Component { - static template = xml``; + static template = xml``; static components = { Child }; state = useState({ b: true }); - child = useRef("child"); } const widget = await mount(Parent, fixture); @@ -258,7 +260,7 @@ describe("style and class handling", () => { await nextTick(); expect(span.className).toBe("c d a"); - (widget.child.comp as Child).state.d = false; + child!.state.d = false; await nextTick(); expect(span.className).toBe("c a"); @@ -266,7 +268,7 @@ describe("style and class handling", () => { await nextTick(); expect(span.className).toBe("c a b"); - (widget.child.comp as Child).state.d = true; + child!.state.d = true; await nextTick(); expect(span.className).toBe("c a b d"); });