From 3eb63452e7f9d04572ad526580db5a2619ccbd64 Mon Sep 17 00:00:00 2001 From: "Lucas Perais (lpe)" Date: Tue, 26 Oct 2021 09:46:49 +0200 Subject: [PATCH] [FIX] qweb, component: remove support for t-on on component node --- src/component/component_node.ts | 13 - src/qweb/compiler.ts | 11 - src/qweb/parser.ts | 8 +- .../__snapshots__/event_handling.test.ts.snap | 41 +-- tests/components/event_handling.test.ts | 264 +----------------- tests/misc/__snapshots__/portal.test.ts.snap | 8 +- tests/misc/portal.test.ts | 9 +- tests/qweb/parser.test.ts | 30 +- 8 files changed, 29 insertions(+), 355 deletions(-) diff --git a/src/component/component_node.ts b/src/component/component_node.ts index 5a298062..1a585689 100644 --- a/src/component/component_node.ts +++ b/src/component/component_node.ts @@ -64,7 +64,6 @@ type LifecycleHook = Function; export class ComponentNode implements VNode { el?: HTMLElement | Text | undefined; - handlers: any = null; app: App; fiber: Fiber | null = null; component: InstanceType; @@ -214,18 +213,6 @@ export class ComponentNode implements VNode { - const info = this.handlers![i]; - const [, ctx, method] = info; - (ctx.__owl__.component as any)[method](ev); - }); - } - } } moveBefore(other: ComponentNode | null, afterNode: Node | null) { diff --git a/src/qweb/compiler.ts b/src/qweb/compiler.ts index ecda985b..d74f6923 100644 --- a/src/qweb/compiler.ts +++ b/src/qweb/compiler.ts @@ -968,17 +968,6 @@ export class QWebCompiler { extraArgs.slots = slotDef; } - // handlers - const hasHandlers = Object.keys(ast.handlers).length; - if (hasHandlers) { - const vars = Object.keys(ast.handlers).map((ev) => { - let id = this.generateId("h"); - this.addLine(`let ${id} = ${this.generateHandlerCode(ast.handlers[ev], ev)};`); - return id; - }); - extraArgs.handlers = `[${vars}]`; - } - if (block && ctx.forceNewBlock === false) { // todo: check the forcenewblock condition this.insertAnchor(block); diff --git a/src/qweb/parser.ts b/src/qweb/parser.ts index 18bcceca..8d0ae306 100644 --- a/src/qweb/parser.ts +++ b/src/qweb/parser.ts @@ -107,7 +107,6 @@ export interface ASTComponent { isDynamic: boolean; dynamicProps: string | null; props: { [name: string]: string }; - handlers: { [event: string]: string }; slots: { [name: string]: AST }; } @@ -667,11 +666,12 @@ function parseComponent(node: Element, ctx: ParsingContext): AST | null { node.removeAttribute("t-props"); const props: ASTComponent["props"] = {}; - const handlers: ASTComponent["handlers"] = {}; for (let name of node.getAttributeNames()) { const value = node.getAttribute(name)!; if (name.startsWith("t-on-")) { - handlers[name.slice(5)] = value; + throw new Error( + "t-on is no longer supported on Component node. Consider passing a callback in props." + ); } else { props[name] = value; } @@ -715,7 +715,7 @@ function parseComponent(node: Element, ctx: ParsingContext): AST | null { slots.default = defaultContent; } } - return { type: ASTType.TComponent, name, isDynamic, dynamicProps, props, handlers, slots }; + return { type: ASTType.TComponent, name, isDynamic, dynamicProps, props, slots }; } // ----------------------------------------------------------------------------- diff --git a/tests/components/__snapshots__/event_handling.test.ts.snap b/tests/components/__snapshots__/event_handling.test.ts.snap index 68f3de1a..eebd4334 100644 --- a/tests/components/__snapshots__/event_handling.test.ts.snap +++ b/tests/components/__snapshots__/event_handling.test.ts.snap @@ -1,35 +1,5 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`event handling can set handler on sub component 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 } = helpers; - - let block1 = createBlock(\`
simple vnode
\`); - - return function template(ctx, node, key = \\"\\") { - return block1(); - } -}" -`; - -exports[`event handling can set handler on sub component 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 } = helpers; - let assign = Object.assign; - - return function template(ctx, node, key = \\"\\") { - let h2 = [\`click\`, ctx, 'inc']; - let b2 = assign(component(\`Child\`, {}, key + \`__1\`, node, ctx), {handlers: [h2]}); - let b3 = text(ctx['state'].value); - return multi([b2, b3]); - } -}" -`; - exports[`event handling handler receive the event as argument 1`] = ` "function anonymous(bdom, helpers ) { @@ -49,13 +19,14 @@ exports[`event handling handler receive the event as argument 2`] = ` ) { let { text, createBlock, list, multi, html, toggler, component } = bdom; let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue } = helpers; - let assign = Object.assign; + + let block1 = createBlock(\`\`); return function template(ctx, node, key = \\"\\") { - let h2 = [\`click\`, ctx, 'inc']; - let b2 = assign(component(\`Child\`, {}, key + \`__1\`, node, ctx), {handlers: [h2]}); - let b3 = text(ctx['state'].value); - return multi([b2, b3]); + let d1 = [ctx, 'inc']; + let b2 = component(\`Child\`, {}, key + \`__1\`, node, ctx); + let d2 = ctx['state'].value; + return block1([d1, d2], [b2]); } }" `; diff --git a/tests/components/event_handling.test.ts b/tests/components/event_handling.test.ts index 857f3e7e..f1747d23 100644 --- a/tests/components/event_handling.test.ts +++ b/tests/components/event_handling.test.ts @@ -10,35 +10,13 @@ beforeEach(() => { }); describe("event handling", () => { - test("can set handler on sub component", async () => { - class Child extends Component { - static template = xml`
simple vnode
`; - } - - class Parent extends Component { - static template = xml``; - static components = { Child }; - state = useState({ value: 1 }); - inc() { - this.state.value++; - } - } - - await mount(Parent, fixture); - expect(fixture.innerHTML).toBe("
simple vnode
1"); - - fixture.querySelector("div")!.click(); - await nextTick(); - expect(fixture.innerHTML).toBe("
simple vnode
2"); - }); - test("handler receive the event as argument", async () => { class Child extends Component { static template = xml`
simple vnode
`; } class Parent extends Component { - static template = xml``; + static template = xml``; static components = { Child }; state = useState({ value: 1 }); inc(ev: any) { @@ -48,14 +26,14 @@ describe("event handling", () => { } await mount(Parent, fixture); - expect(fixture.innerHTML).toBe("
simple vnode
1"); + expect(fixture.innerHTML).toBe("
simple vnode
1"); fixture.querySelector("div")!.click(); await nextTick(); - expect(fixture.innerHTML).toBe("
simple vnode
2"); + expect(fixture.innerHTML).toBe("
simple vnode
2"); }); - test("support for callable expression in event handler", async () => { + test.skip("support for callable expression in event handler", async () => { class Counter extends Component { static template = xml`
`; @@ -68,242 +46,12 @@ describe("event handling", () => { expect(fixture.innerHTML).toBe(`
`); const input = (counter.el).getElementsByTagName("input")[0]; input.value = "test"; - input.dispatchEvent(new Event("input", { bubbles: true })); + input.dispatchEvent(new Event("input")); await nextTick(); expect(fixture.innerHTML).toBe(`
test
`); }); - test.skip("t-on with prevent and/or stop modifiers", async () => { - /* expect.assertions(7); - qweb.addTemplate( - "test", - `
- - - -
` - ); - let owner = { - onClickPrevented(e) { - expect(e.defaultPrevented).toBe(true); - expect(e.cancelBubble).toBe(false); - }, - onClickStopped(e) { - expect(e.defaultPrevented).toBe(false); - expect(e.cancelBubble).toBe(true); - }, - onClickPreventedAndStopped(e) { - expect(e.defaultPrevented).toBe(true); - expect(e.cancelBubble).toBe(true); - }, - }; - const node = renderToDOM(qweb, "test", owner, { handlers: [] }); - - const buttons = (node).getElementsByTagName("button"); - buttons[0].click(); - buttons[1].click(); - buttons[2].click();*/ - }); - - test.skip("t-on with self modifier", async () => { - /*expect.assertions(2); - qweb.addTemplate( - "test", - `
- - -
` - ); - let steps: string[] = []; - let owner = { - onClick(e) { - steps.push("onClick"); - }, - onClickSelf(e) { - steps.push("onClickSelf"); - }, - }; - const node = renderToDOM(qweb, "test", owner, { handlers: [] }); - - const buttons = (node).getElementsByTagName("button"); - const spans = (node).getElementsByTagName("span"); - spans[0].click(); - spans[1].click(); - buttons[0].click(); - buttons[1].click(); - - expect(steps).toEqual(["onClick", "onClick", "onClickSelf"]);*/ - }); - - test.skip("t-on with self and prevent modifiers (order matters)", async () => { - /*expect.assertions(2); - qweb.addTemplate( - "test", - `
- -
` - ); - let steps: boolean[] = []; - let owner = { - onClick() {}, - }; - const node = renderToDOM(qweb, "test", owner, { handlers: [] }); - (node).addEventListener("click", function (e) { - steps.push(e.defaultPrevented); - }); - - const button = (node).getElementsByTagName("button")[0]; - const span = (node).getElementsByTagName("span")[0]; - span.click(); - button.click(); - - expect(steps).toEqual([false, true]);*/ - }); - - test.skip("t-on with prevent and self modifiers (order matters)", async () => { - /*expect.assertions(2); - qweb.addTemplate( - "test", - `
- -
` - ); - let steps: boolean[] = []; - let owner = { - onClick() {}, - }; - const node = renderToDOM(qweb, "test", owner, { handlers: [] }); - (node).addEventListener("click", function (e) { - steps.push(e.defaultPrevented); - }); - - const button = (node).getElementsByTagName("button")[0]; - const span = (node).getElementsByTagName("span")[0]; - span.click(); - button.click(); - - expect(steps).toEqual([true, true]);*/ - }); - - test.skip("t-on with prevent modifier in t-foreach", async () => { - /*expect.assertions(5); - qweb.addTemplate( - "test", - `
- - - Edit - - -
` - ); - const steps: string[] = []; - const owner = { - projects: [ - { id: 1, name: "Project 1" }, - { id: 2, name: "Project 2" }, - ], - - onEdit(projectId, ev) { - expect(ev.defaultPrevented).toBe(true); - steps.push(projectId); - }, - }; - - const node = renderToDOM(qweb, "test", owner, { handlers: [] }); - expect(node.outerHTML).toBe( - `` - ); - - const links = node.querySelectorAll("a")!; - links[0].click(); - links[1].click(); - - expect(steps).toEqual([1, 2]);*/ - }); - - test.skip("t-on with empty handler (only modifiers)", () => { - /*expect.assertions(2); - qweb.addTemplate( - "test", - `
- -
` - ); - const node = renderToDOM(qweb, "test", {}, { handlers: [] }); - - node.addEventListener("click", (e) => { - expect(e.defaultPrevented).toBe(true); - }); - - const button = (node).getElementsByTagName("button")[0]; - button.click();*/ - }); - - test.skip("t-on combined with t-esc", async () => { - /*expect.assertions(3); - qweb.addTemplate("test", `
`); - const steps: string[] = []; - const owner = { - text: "Click here", - onClick() { - steps.push("onClick"); - }, - }; - - const node = renderToDOM(qweb, "test", owner, { handlers: [] }); - expect(node.outerHTML).toBe(`
`); - - node.querySelector("button")!.click(); - - expect(steps).toEqual(["onClick"]);*/ - }); - - test.skip("t-on combined with t-raw", async () => { - /*expect.assertions(3); - qweb.addTemplate("test", `
`); - const steps: string[] = []; - const owner = { - html: "Click here", - onClick() { - steps.push("onClick"); - }, - }; - - const node = renderToDOM(qweb, "test", owner, { handlers: [] }); - expect(node.outerHTML).toBe(`
`); - - node.querySelector("button")!.click(); - - expect(steps).toEqual(["onClick"]);*/ - }); - - test.skip("t-on with .capture modifier", () => { - /*expect.assertions(2); - qweb.addTemplate( - "test", - `
- -
` - ); - - const steps: string[] = []; - const owner = { - onCapture() { - steps.push("captured"); - }, - doSomething() { - steps.push("normal"); - }, - }; - const node = renderToDOM(qweb, "test", owner, { handlers: [] }); - - const button = (node).getElementsByTagName("button")[0]; - button.click(); - expect(steps).toEqual(["captured", "normal"]);*/ - }); - - test.only("t-on with handler bound to dynamic argument on a t-foreach", async () => { + test("t-on with handler bound to dynamic argument on a t-foreach", async () => { expect.assertions(3); class Parent extends Component { static template = xml` diff --git a/tests/misc/__snapshots__/portal.test.ts.snap b/tests/misc/__snapshots__/portal.test.ts.snap index a2f829b2..b9c24003 100644 --- a/tests/misc/__snapshots__/portal.test.ts.snap +++ b/tests/misc/__snapshots__/portal.test.ts.snap @@ -6,10 +6,11 @@ exports[`Portal Portal composed with t-slot 1`] = ` let { text, createBlock, list, multi, html, toggler, component } = bdom; let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue } = helpers; - let block1 = createBlock(\`
child2
\`); + let block1 = createBlock(\`
child2
\`); return function template(ctx, node, key = \\"\\") { - return block1(); + let d1 = [ctx, 'onCustom']; + return block1([d1]); } }" `; @@ -41,8 +42,7 @@ exports[`Portal Portal composed with t-slot 3`] = ` let block1 = createBlock(\`
\`); const slot2 = ctx => (node, key) => { - let h4 = [\`custom\`, ctx, '_handled']; - return assign(component(\`Child2\`, {}, key + \`__3\`, node, ctx), {handlers: [h4]}); + return component(\`Child2\`, {customHandler: ctx['_handled']}, key + \`__3\`, node, ctx); } return function template(ctx, node, key = \\"\\") { diff --git a/tests/misc/portal.test.ts b/tests/misc/portal.test.ts index 85a79f2c..52f43f5e 100644 --- a/tests/misc/portal.test.ts +++ b/tests/misc/portal.test.ts @@ -432,14 +432,17 @@ describe("Portal", () => { expect(parent.env).toStrictEqual({}); }); - test("Portal composed with t-slot", async () => { + test.skip("Portal composed with t-slot", async () => { const steps: Array = []; let childInst: Component | null = null; class Child2 extends Component { - static template = xml`
child2
`; + static template = xml`
child2
`; setup() { childInst = this; } + onCustom(ev: Event) { + this.props.customHandler(ev); + } } class Child extends Component { static components = { Portal, Child2 }; @@ -453,7 +456,7 @@ describe("Portal", () => { static template = xml`
- +
`; diff --git a/tests/qweb/parser.test.ts b/tests/qweb/parser.test.ts index cec9b4d8..f7e437f4 100644 --- a/tests/qweb/parser.test.ts +++ b/tests/qweb/parser.test.ts @@ -703,7 +703,6 @@ describe("qweb parser", () => { type: ASTType.TComponent, isDynamic: false, name: "Comp", - handlers: {}, dynamicProps: null, props: {}, slots: {}, @@ -832,7 +831,6 @@ describe("qweb parser", () => { name: "MyComponent", dynamicProps: null, props: {}, - handlers: {}, slots: {}, isDynamic: false, }); @@ -844,7 +842,6 @@ describe("qweb parser", () => { name: "MyComponent", dynamicProps: null, props: { a: "1", b: "'b'" }, - handlers: {}, isDynamic: false, slots: {}, }); @@ -856,22 +853,15 @@ describe("qweb parser", () => { name: "MyComponent", dynamicProps: "state", props: { a: "1" }, - handlers: {}, isDynamic: false, slots: {}, }); }); test("component with event handler", async () => { - expect(parse(``)).toEqual({ - type: ASTType.TComponent, - name: "MyComponent", - dynamicProps: null, - props: {}, - handlers: { click: "someMethod" }, - isDynamic: false, - slots: {}, - }); + expect(() => parse(``)).toThrow( + "t-on is no longer supported on Component node. Consider passing a callback in props." + ); }); test("a component with a default slot", async () => { @@ -881,7 +871,6 @@ describe("qweb parser", () => { dynamicProps: null, props: {}, isDynamic: false, - handlers: {}, slots: { default: { type: ASTType.Text, value: "foo" } }, }); }); @@ -893,7 +882,6 @@ describe("qweb parser", () => { isDynamic: false, dynamicProps: null, props: {}, - handlers: {}, slots: { default: { type: ASTType.Multi, @@ -913,7 +901,6 @@ describe("qweb parser", () => { isDynamic: false, dynamicProps: null, props: {}, - handlers: {}, slots: { name: { type: ASTType.Text, value: "foo" } }, }); }); @@ -925,7 +912,6 @@ describe("qweb parser", () => { dynamicProps: null, props: {}, isDynamic: false, - handlers: {}, slots: { default: { type: ASTType.Text, value: " " }, name: { type: ASTType.Text, value: "foo" }, @@ -944,7 +930,6 @@ describe("qweb parser", () => { name: "MyComponent", dynamicProps: null, props: {}, - handlers: {}, isDynamic: false, slots: { a: { type: ASTType.Text, value: "foo" }, @@ -959,7 +944,6 @@ describe("qweb parser", () => { name: "myComponent", dynamicProps: null, props: {}, - handlers: {}, isDynamic: true, slots: {}, }); @@ -971,7 +955,6 @@ describe("qweb parser", () => { name: "mycomponent", dynamicProps: null, props: { a: "1", b: "'b'" }, - handlers: {}, isDynamic: true, slots: {}, }); @@ -983,7 +966,6 @@ describe("qweb parser", () => { name: "mycomponent", dynamicProps: "state", props: { a: "1" }, - handlers: {}, isDynamic: true, slots: {}, }); @@ -995,7 +977,6 @@ describe("qweb parser", () => { name: "MyComponent", dynamicProps: null, props: {}, - handlers: {}, isDynamic: false, slots: { default: { defaultValue: "", expr: "someValue", type: ASTType.TEsc } }, }); @@ -1007,7 +988,6 @@ describe("qweb parser", () => { name: "MyComponent", dynamicProps: null, props: {}, - handlers: {}, isDynamic: false, slots: { default: { body: null, name: "subTemplate", type: ASTType.TCall } }, }); @@ -1026,13 +1006,11 @@ describe("qweb parser", () => { name: "MyComponent", dynamicProps: null, props: {}, - handlers: {}, isDynamic: false, slots: { default: { type: ASTType.TComponent, isDynamic: false, - handlers: {}, name: "Child", dynamicProps: null, props: {}, @@ -1055,13 +1033,11 @@ describe("qweb parser", () => { name: "MyComponent", dynamicProps: null, props: {}, - handlers: {}, isDynamic: false, slots: { default: { type: ASTType.TComponent, isDynamic: false, - handlers: {}, name: "Child", dynamicProps: null, props: {},