From 0bc9573a8a3af4e6cec22b1460542ecf1ce1bad7 Mon Sep 17 00:00:00 2001 From: Samuel Degueldre Date: Tue, 7 Dec 2021 15:09:21 +0100 Subject: [PATCH] [FIX] component: make arrow-function capture backwards compatible When fixing the absence of capture for arrow functions passed as props, we unintentionally introduced a breaking change: bare function calls in the arrow functions used to be called with the rendering context as their this value and this was no longer the case. This commit fixes that by intentionally not capturing the value of functions that are called withing the arrow function. --- src/qweb/compilation_context.ts | 11 +++- .../__snapshots__/component.test.ts.snap | 55 +++++++++++++++++++ tests/component/component.test.ts | 43 +++++++++++++++ tests/qweb/__snapshots__/qweb.test.ts.snap | 3 +- 4 files changed, 108 insertions(+), 4 deletions(-) diff --git a/src/qweb/compilation_context.ts b/src/qweb/compilation_context.ts index 671ff6b1..d3cb8e20 100644 --- a/src/qweb/compilation_context.ts +++ b/src/qweb/compilation_context.ts @@ -162,7 +162,7 @@ export class CompilationContext { const tokens = compileExprToArray(expr, this.variables); const done = new Set(); return tokens - .map((tok) => { + .map((tok, i) => { // "this" in captured expressions should be the current component if (tok.value === "this") { if (!done.has("this")) { @@ -174,7 +174,14 @@ export class CompilationContext { // Variables that should be looked up in the scope. isLocal is for arrow // function arguments that should stay untouched (eg "ev => ev" should // not become "const ev_1 = scope['ev']; ev_1 => ev_1") - if (tok.varName && !tok.isLocal) { + if ( + tok.varName && + !tok.isLocal && + // HACK: for backwards compatibility, we don't capture bare methods + // this allows them to be called with the rendering context/scope + // as their this value. + (!tokens[i + 1] || tokens[i + 1].type !== "LEFT_PAREN") + ) { if (!done.has(tok.varName)) { done.add(tok.varName); this.addLine(`const ${tok.varName}_${argId} = ${tok.value};`); diff --git a/tests/component/__snapshots__/component.test.ts.snap b/tests/component/__snapshots__/component.test.ts.snap index c712eeda..b3e4e0d2 100644 --- a/tests/component/__snapshots__/component.test.ts.snap +++ b/tests/component/__snapshots__/component.test.ts.snap @@ -1798,6 +1798,61 @@ exports[`props evaluation arrow function prop captures loop variables 2`] = ` }" `; +exports[`props evaluation bare function calls in arrow function has rendering context as 'this' 1`] = ` +"function anonymous(context, extra +) { + // Template name: \\"Parent\\" + let utils = this.constructor.utils; + let QWeb = this.constructor; + let parent = context; + let scope = Object.create(context); + let h = this.h; + let c1 = [], p1 = {key:1}; + let vn1 = h('div', p1, c1); + scope.ctxVal = 2; + // Component 'Child' + let w3 = '__4__' in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap['__4__']] : false; + let props3 = {callback:value=>scope['setValue'](value),value:scope['state'].val}; + if (w3 && w3.__owl__.currentFiber && !w3.__owl__.vnode) { + w3.destroy(); + w3 = false; + } + if (w3) { + w3.__updateProps(props3, extra.fiber, undefined); + let pvnode = w3.__owl__.pvnode; + c1.push(pvnode); + } else { + let componentKey3 = \`Child\`; + let W3 = scope['Child'] || context.constructor.components[componentKey3] || QWeb.components[componentKey3]; + if (!W3) {throw new Error('Cannot find the definition of component \\"' + componentKey3 + '\\"')} + w3 = new W3(parent, props3); + parent.__owl__.cmap['__4__'] = w3.__owl__.id; + let fiber = w3.__prepare(extra.fiber, undefined, () => { const vnode = fiber.vnode; pvnode.sel = vnode.sel; }); + let pvnode = h('dummy', {key: '__4__', hook: {remove() {},destroy(vn) {w3.destroy();}}}); + c1.push(pvnode); + w3.__owl__.pvnode = pvnode; + } + w3.__owl__.parentLastFiberId = extra.fiber.id; + return vn1; +}" +`; + +exports[`props evaluation bare function calls in arrow function has rendering context as 'this' 2`] = ` +"function anonymous(context, extra +) { + // Template name: \\"Child\\" + let scope = Object.create(context); + let h = this.h; + let c5 = [], p5 = {key:5}; + let vn5 = h('span', p5, c5); + let _6 = scope['props'].value; + if (_6 != null) { + c5.push({text: _6}); + } + return vn5; +}" +`; + exports[`props evaluation t-set with a body expression can be used as textual prop 1`] = ` "function anonymous(context, extra ) { diff --git a/tests/component/component.test.ts b/tests/component/component.test.ts index f08fa452..25868bdd 100644 --- a/tests/component/component.test.ts +++ b/tests/component/component.test.ts @@ -1920,6 +1920,49 @@ describe("props evaluation ", () => { expect(env.qweb.templates.Child.fn.toString()).toMatchSnapshot(); }); + test("bare function calls in arrow function has rendering context as 'this'", async () => { + expect.assertions(7); + let child, parent; + class Child extends Component { + setup() { + child = this; + } + } + env.qweb.addTemplate("Child", ``); + + class Parent extends Component { + static components = { Child }; + state = useState({ val: 42 }); + setup() { + parent = this; + } + setValue(value) { + // 'this' is the rendering context, NOT the instance + expect(this).not.toBe(parent); + // the state in the rendering context should be the same as the instance's + expect(this.state).toBe(parent.state); + expect((this as any).ctxVal).toBe(2); + this.state.val = value; + } + } + env.qweb.addTemplate( + "Parent", + `
+ + +
` + ); + + const widget = new Parent(); + await widget.mount(fixture); + expect(fixture.innerHTML).toBe("
42
"); + child.props.callback(123); + await nextTick(); + expect(fixture.innerHTML).toBe("
123
"); + expect(env.qweb.templates.Parent.fn.toString()).toMatchSnapshot(); + expect(env.qweb.templates.Child.fn.toString()).toMatchSnapshot(); + }); + test("arrow function prop captures context component instance as 'this' inside slot", async () => { expect.assertions(7); let child, parent; diff --git a/tests/qweb/__snapshots__/qweb.test.ts.snap b/tests/qweb/__snapshots__/qweb.test.ts.snap index 60e1d0f5..a783d931 100644 --- a/tests/qweb/__snapshots__/qweb.test.ts.snap +++ b/tests/qweb/__snapshots__/qweb.test.ts.snap @@ -3111,8 +3111,7 @@ exports[`t-on t-on with inline statement, part 3 1`] = ` let c1 = [], p1 = {key:1,on:{}}; let vn1 = h('button', p1, c1); const state_2 = scope['state']; - const someFunction_2 = scope['someFunction']; - p1.on['click'] = function (e) {if (context.__owl__.status === 5){return}const res = (() => { return state_2.n=someFunction_2(3) })(); if (typeof res === 'function') { res(e) }}; + p1.on['click'] = function (e) {if (context.__owl__.status === 5){return}const res = (() => { return state_2.n=scope['someFunction'](3) })(); if (typeof res === 'function') { res(e) }}; c1.push({text: \`Toggle\`}); return vn1; }"