From df59ec49aefce2e0913fdc1792d42b9680fb28b6 Mon Sep 17 00:00:00 2001 From: Samuel Degueldre Date: Fri, 2 Dec 2022 13:37:51 +0100 Subject: [PATCH] [FIX] t-call-context: make `this` unavailable in rendering context t-call-context is a feature that's supposed to mask the rendering context completely, but currently the component remains available through `this`. This commit stops treating `this` as a reserved word, so that it's compiled to a lookup in the rendering context, and adds `this` to the rendering context when binding the component's rendering function. With these changes, `this` behaves the same as before when outside a t-call-context, but when the rendering context is overriden, the template can no longer access `this`. `this` still represents the instance of the component inside of the rendering function since it's needed by owl internally. A side-effect of this change is that now the rendering context is no longer the instance of the component by default, but is always an object with the component in its prototype chain. This was already the case before in some contexts (eg inside t-foreach, or inside components with a t-set/t-call anywhere in its template). This can cause issues in rare cases when a component method was called directly on the rendering context, as before this change, the method's bound this would be the component instance (except in a t-foreach, component with a t-set/t-call, etc), while after this change it is now never the component instance. When the method only reads on `this` there is no issue as all the components properties are available on the rendering contexts, but setting a value on `this` will write on the rendering context and not the component which is likely a mistake. While this is a breaking change, simply adding a t-set/t-call to any template would break components that would be broken by this change, with this in mind we decided to make this change anyway so that developers get the error as early as possible in the development cycle rather than having a seemingly inocuous change break code under them. --- doc/learning/tutorial_todoapp.md | 6 +-- doc/reference/templates.md | 7 ++- src/compiler/inline_expressions.ts | 2 +- src/runtime/component_node.ts | 3 +- .../__snapshots__/event_handling.test.ts.snap | 5 ++- tests/compiler/event_handling.test.ts | 3 ++ .../__snapshots__/error_handling.test.ts.snap | 22 ++++----- .../__snapshots__/slots.test.ts.snap | 6 ++- .../__snapshots__/style_class.test.ts.snap | 2 +- .../__snapshots__/t_call.test.ts.snap | 45 ++++++++++++++----- .../__snapshots__/t_on.test.ts.snap | 21 +++++---- tests/components/t_call.test.ts | 19 +++++++- tests/misc/__snapshots__/portal.test.ts.snap | 2 +- 13 files changed, 97 insertions(+), 46 deletions(-) diff --git a/doc/learning/tutorial_todoapp.md b/doc/learning/tutorial_todoapp.md index 17a85a55..7f606a0a 100644 --- a/doc/learning/tutorial_todoapp.md +++ b/doc/learning/tutorial_todoapp.md @@ -174,9 +174,9 @@ class Root extends Component { ``` The template contains a [`t-foreach`](../reference/templates.md#loops) loop to iterate -through the tasks. It can find the `tasks` list from the component, since the -component is the rendering context. Note that we use the `id` of each task as a -`t-key`, which is very common. There are two css classes: `task-list` and `task`, +through the tasks. It can find the `tasks` list from the component, since the rendering +context contains the properties of the component. Note that we use the `id` of each task +as a `t-key`, which is very common. There are two css classes: `task-list` and `task`, that we will use in the next section. Finally, notice the use of the `t-att-checked` attribute: diff --git a/doc/reference/templates.md b/doc/reference/templates.md index 1cd613ca..73143d66 100644 --- a/doc/reference/templates.md +++ b/doc/reference/templates.md @@ -115,7 +115,7 @@ It is useful to explain the various rules that apply on these expressions:

NOT valid

``` -2. it can use anything in the rendering context (typically, the component): +2. it can use anything in the rendering context (which typically contains the properties of the component): ```xml

Happy bithday!

@@ -541,9 +541,8 @@ This can be used to define variables scoped to a sub template: ``` Note: by default, the rendering context for a sub template is simply the current -rendering context (so, the current component). However, it may be useful to be -able to specify a specific object as context. This can be done by using the -`t-call-context` directive: +rendering context. However, it may be useful to be able to specify a specific +object as context. This can be done by using the `t-call-context` directive: ```xml diff --git a/src/compiler/inline_expressions.ts b/src/compiler/inline_expressions.ts index adcde781..d9e866b1 100644 --- a/src/compiler/inline_expressions.ts +++ b/src/compiler/inline_expressions.ts @@ -28,7 +28,7 @@ import { OwlError } from "../runtime/error_handling"; //------------------------------------------------------------------------------ const RESERVED_WORDS = - "true,false,NaN,null,undefined,debugger,console,window,in,instanceof,new,function,return,this,eval,void,Math,RegExp,Array,Object,Date".split( + "true,false,NaN,null,undefined,debugger,console,window,in,instanceof,new,function,return,eval,void,Math,RegExp,Array,Object,Date".split( "," ); diff --git a/src/runtime/component_node.ts b/src/runtime/component_node.ts index aa4ec0c3..d94452ad 100644 --- a/src/runtime/component_node.ts +++ b/src/runtime/component_node.ts @@ -114,7 +114,8 @@ export class ComponentNode

implements VNodelucas

\`); return function template(ctx, node, key = \\"\\") { - const v1 = ctx['value']; - let hdlr1 = [()=>this.update(v1), ctx]; + const v1 = ctx['this']; + const v2 = ctx['value']; + let hdlr1 = [()=>v1.update(v2), ctx]; return block1([hdlr1]); } }" diff --git a/tests/compiler/event_handling.test.ts b/tests/compiler/event_handling.test.ts index c79e2d08..11b98738 100644 --- a/tests/compiler/event_handling.test.ts +++ b/tests/compiler/event_handling.test.ts @@ -284,6 +284,9 @@ describe("t-on", () => { expect(this).toBe(owner); expect(val).toBe(444); }, + get this() { + return owner; + }, value: 444, }; diff --git a/tests/components/__snapshots__/error_handling.test.ts.snap b/tests/components/__snapshots__/error_handling.test.ts.snap index c61822c0..4cea9784 100644 --- a/tests/components/__snapshots__/error_handling.test.ts.snap +++ b/tests/components/__snapshots__/error_handling.test.ts.snap @@ -940,9 +940,10 @@ exports[`can catch errors catching error, rethrow, render parent -- a main comp for (let i1 = 0; i1 < l_block1; i1++) { ctx[\`cp\`] = v_block1[i1]; const key1 = ctx['cp'].id; - const v1 = ctx['cp']; + const v1 = ctx['this']; + const v2 = ctx['cp']; const ctx1 = capture(ctx); - c_block1[i1] = withKey(comp2({onError: ()=>this.cleanUp(v1.id),slots: markRaw({'default': {__render: slot1.bind(this), __ctx: ctx1}})}, key + \`__2__\${key1}\`, node, this, null), key1); + c_block1[i1] = withKey(comp2({onError: ()=>v1.cleanUp(v2.id),slots: markRaw({'default': {__render: slot1.bind(this), __ctx: ctx1}})}, key + \`__2__\${key1}\`, node, this, null), key1); } return list(c_block1); } @@ -1014,13 +1015,14 @@ exports[`can catch errors catching in child makes parent render 1`] = ` return function template(ctx, node, key = \\"\\") { ctx = Object.create(ctx); - const [k_block1, v_block1, l_block1, c_block1] = prepareList(Object.entries(this.elements));; + const [k_block1, v_block1, l_block1, c_block1] = prepareList(Object.entries(ctx['this'].elements));; for (let i1 = 0; i1 < l_block1; i1++) { ctx[\`elem\`] = v_block1[i1]; const key1 = ctx['elem'][0]; - const v1 = ctx['elem']; + const v1 = ctx['this']; + const v2 = ctx['elem']; const ctx1 = capture(ctx); - c_block1[i1] = withKey(comp2({onError: (_error)=>this.onError(v1[0],_error),slots: markRaw({'default': {__render: slot1.bind(this), __ctx: ctx1}})}, key + \`__2__\${key1}\`, node, this, null), key1); + c_block1[i1] = withKey(comp2({onError: (_error)=>v1.onError(v2[0],_error),slots: markRaw({'default': {__render: slot1.bind(this), __ctx: ctx1}})}, key + \`__2__\${key1}\`, node, this, null), key1); } return list(c_block1); } @@ -1155,7 +1157,7 @@ exports[`can catch errors onError in class inheritance is called if rethrown 2`] return function template(ctx, node, key = \\"\\") { let b2,b3; if (!ctx['state'].error) { - b2 = text(this.will.crash); + b2 = text(ctx['this'].will.crash); } else { b3 = text(ctx['state'].error); } @@ -1186,7 +1188,7 @@ exports[`can catch errors onError in class inheritance is not called if no rethr return function template(ctx, node, key = \\"\\") { let b2,b3; if (!ctx['state'].error) { - b2 = text(this.will.crash); + b2 = text(ctx['this'].will.crash); } else { b3 = text(ctx['state'].error); } @@ -1218,7 +1220,7 @@ exports[`errors and promises a rendering error in a sub component will reject th let block1 = createBlock(\`
\`); return function template(ctx, node, key = \\"\\") { - let txt1 = this.will.crash; + let txt1 = ctx['this'].will.crash; return block1([txt1]); } }" @@ -1232,7 +1234,7 @@ exports[`errors and promises a rendering error will reject the mount promise 1`] let block1 = createBlock(\`
\`); return function template(ctx, node, key = \\"\\") { - let txt1 = this.will.crash; + let txt1 = ctx['this'].will.crash; return block1([txt1]); } }" @@ -1277,7 +1279,7 @@ exports[`errors and promises a rendering error will reject the render promise 1` return function template(ctx, node, key = \\"\\") { let b2; if (ctx['flag']) { - b2 = text(this.will.crash); + b2 = text(ctx['this'].will.crash); } return block1([], [b2]); } diff --git a/tests/components/__snapshots__/slots.test.ts.snap b/tests/components/__snapshots__/slots.test.ts.snap index 4eab0079..cf61d22a 100644 --- a/tests/components/__snapshots__/slots.test.ts.snap +++ b/tests/components/__snapshots__/slots.test.ts.snap @@ -1992,7 +1992,8 @@ exports[`slots slot content is bound to caller (variation) 1`] = ` ctx = Object.create(ctx); ctx[isBoundary] = 1 setContextValue(ctx, \\"var\\", 1); - let hdlr1 = [()=>this.inc(), ctx]; + const v1 = ctx['this']; + let hdlr1 = [()=>v1.inc(), ctx]; return block1([hdlr1]); } @@ -2359,7 +2360,8 @@ exports[`slots slots are properly bound to correct component 2`] = ` ctx = Object.create(ctx); ctx[isBoundary] = 1 setContextValue(ctx, \\"var\\", 1); - let hdlr1 = [()=>this.increment(), ctx]; + const v1 = ctx['this']; + let hdlr1 = [()=>v1.increment(), ctx]; let txt1 = ctx['state'].value; return block1([hdlr1, txt1]); } diff --git a/tests/components/__snapshots__/style_class.test.ts.snap b/tests/components/__snapshots__/style_class.test.ts.snap index cafd236a..77665f4d 100644 --- a/tests/components/__snapshots__/style_class.test.ts.snap +++ b/tests/components/__snapshots__/style_class.test.ts.snap @@ -363,7 +363,7 @@ exports[`style and class handling error in subcomponent with class 2`] = ` return function template(ctx, node, key = \\"\\") { let attr1 = ctx['props'].class; - let txt1 = this.will.crash; + let txt1 = ctx['this'].will.crash; return block1([attr1, txt1]); } }" diff --git a/tests/components/__snapshots__/t_call.test.ts.snap b/tests/components/__snapshots__/t_call.test.ts.snap index f56af038..7cffa0ae 100644 --- a/tests/components/__snapshots__/t_call.test.ts.snap +++ b/tests/components/__snapshots__/t_call.test.ts.snap @@ -109,7 +109,8 @@ exports[`t-call handlers are properly bound through a dynamic t-call 2`] = ` let block1 = createBlock(\`

lucas

\`); return function template(ctx, node, key = \\"\\") { - let hdlr1 = [()=>this.update(), ctx]; + const v1 = ctx['this']; + let hdlr1 = [()=>v1.update(), ctx]; return block1([hdlr1]); } }" @@ -168,8 +169,9 @@ exports[`t-call handlers with arguments are properly bound through a t-call 2`] let block1 = createBlock(\`

lucas

\`); return function template(ctx, node, key = \\"\\") { - const v1 = ctx['a']; - let hdlr1 = [()=>this.update(v1), ctx]; + const v1 = ctx['this']; + const v2 = ctx['a']; + let hdlr1 = [()=>v1.update(v2), ctx]; return block1([hdlr1]); } }" @@ -287,7 +289,7 @@ exports[`t-call recursive t-call binding this -- static t-call 2`] = ` ctx[isBoundary] = 1 let b2; if (ctx['level']<2) { - let hdlr1 = [\\"stop\\", ctx['onClicked'].bind(this), ctx]; + let hdlr1 = [\\"stop\\", ctx['onClicked'].bind(ctx['this']), ctx]; let txt1 = ctx['level']; const b3 = block3([hdlr1, txt1]); ctx = Object.create(ctx); @@ -538,7 +540,6 @@ exports[`t-call t-call-context: ComponentNode is not looked up in the context 2` let block2 = createBlock(\`
outside slot
\`); let block4 = createBlock(\`
I'm the default slot
\`); let block5 = createBlock(\`
\`); - let block6 = createBlock(\`
\`); function slot1(ctx, node, key = \\"\\") { const refs = this.__owl__.refs; @@ -547,11 +548,9 @@ exports[`t-call t-call-context: ComponentNode is not looked up in the context 2` ctx[isBoundary] = 1 const b4 = block4([ref2]); setContextValue(ctx, \\"test\\", 3); - let txt1 = this.__owl__.name; + let txt1 = ctx['test']; const b5 = block5([txt1]); - let txt2 = ctx['test']; - const b6 = block6([txt2]); - return multi([b4, b5, b6]); + return multi([b4, b5]); } return function template(ctx, node, key = \\"\\") { @@ -559,8 +558,8 @@ exports[`t-call t-call-context: ComponentNode is not looked up in the context 2` const ref1 = (el) => refs[\`myRef\`] = el; const b2 = block2([ref1]); const ctx1 = capture(ctx); - const b7 = comp1({prop: bind(this, ctx['method']),slots: markRaw({'default': {__render: slot1.bind(this), __ctx: ctx1}})}, key + \`__1\`, node, this, null); - return multi([b2, b7]); + const b6 = comp1({prop: bind(this, ctx['method']),slots: markRaw({'default': {__render: slot1.bind(this), __ctx: ctx1}})}, key + \`__1\`, node, this, null); + return multi([b2, b6]); } }" `; @@ -624,3 +623,27 @@ exports[`t-call t-call-context: slots don't make component available again when } }" `; + +exports[`t-call t-call-context: this is not available inside t-call-context 1`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + const callTemplate_1 = app.getTemplate(\`someTemplate\`); + + return function template(ctx, node, key = \\"\\") { + let ctx1 = {}; + return callTemplate_1.call(this, ctx1, node, key + \`__1\`); + } +}" +`; + +exports[`t-call t-call-context: this is not available inside t-call-context 2`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + return text(ctx['this']); + } +}" +`; diff --git a/tests/components/__snapshots__/t_on.test.ts.snap b/tests/components/__snapshots__/t_on.test.ts.snap index 5b30afd3..9d710f37 100644 --- a/tests/components/__snapshots__/t_on.test.ts.snap +++ b/tests/components/__snapshots__/t_on.test.ts.snap @@ -111,8 +111,9 @@ exports[`t-on t-on method call in t-foreach 1`] = ` const key1 = ctx['val']; let txt1 = ctx['val_index']; let txt2 = ctx['val']+''; - const v1 = ctx['val']; - let hdlr1 = [()=>this.addVal(v1), ctx]; + const v1 = ctx['this']; + const v2 = ctx['val']; + let hdlr1 = [()=>v1.addVal(v2), ctx]; c_block2[i1] = withKey(block3([txt1, txt2, hdlr1]), key1); } const b2 = list(c_block2); @@ -197,8 +198,9 @@ exports[`t-on t-on on components and t-foreach 1`] = ` for (let i1 = 0; i1 < l_block1; i1++) { ctx[\`name\`] = v_block1[i1]; const key1 = ctx['name']; - const v1 = ctx['name']; - const hdlr1 = [()=>this.log(v1), ctx]; + const v1 = ctx['this']; + const v2 = ctx['name']; + const hdlr1 = [()=>v1.log(v2), ctx]; c_block1[i1] = withKey(catcher1(comp1({value: ctx['name']}, key + \`__1__\${key1}\`, node, this, null), [hdlr1]), key1); } return list(c_block1); @@ -293,8 +295,9 @@ exports[`t-on t-on on components, with a handler update 1`] = ` ctx = Object.create(ctx); ctx[isBoundary] = 1 setContextValue(ctx, \\"name\\", ctx['state'].name); - const v1 = ctx['name']; - const hdlr1 = [()=>this.log(v1), ctx]; + const v1 = ctx['this']; + const v2 = ctx['name']; + const hdlr1 = [()=>v1.log(v2), ctx]; return catcher1(comp1({value: ctx['name']}, key + \`__1\`, node, this, null), [hdlr1]); } }" @@ -393,7 +396,8 @@ exports[`t-on t-on on t-set-slots 1`] = ` function slot1(ctx, node, key = \\"\\") { const b6 = block6(); const b7 = block7(); - const hdlr1 = [()=>this.state.count++, ctx]; + const v1 = ctx['this']; + const hdlr1 = [()=>v1.state.count++, ctx]; return catcher1(multi([b6, b7]), [hdlr1]); } @@ -450,7 +454,8 @@ exports[`t-on t-on on t-slots 2`] = ` const b2 = text(\` [\`); const b3 = text(ctx['state'].count); const b4 = text(\`] \`); - const hdlr1 = [()=>this.state.count++, ctx]; + const v1 = ctx['this']; + const hdlr1 = [()=>v1.state.count++, ctx]; const b5 = catcher1(callSlot(ctx, node, key, 'default', false, {}), [hdlr1]); return multi([b2, b3, b4, b5]); } diff --git a/tests/components/t_call.test.ts b/tests/components/t_call.test.ts index 9a425677..3e70dd6e 100644 --- a/tests/components/t_call.test.ts +++ b/tests/components/t_call.test.ts @@ -345,14 +345,13 @@ describe("t-call", () => {
I'm the default slot
-
`, }); expect(fixture.innerHTML).toBe( - "
outside slot
I'm the default slot
Root
3
" + "
outside slot
I'm the default slot
3
" ); expect(Object.keys(child.__owl__.refs)).toEqual([]); expect(Object.keys(root.__owl__.refs)).toEqual(["myRef", "myRef2"]); @@ -383,4 +382,20 @@ describe("t-call", () => { }); expect(fixture.innerHTML).toBe(""); }); + + test("t-call-context: this is not available inside t-call-context", async () => { + class Root extends Component { + static template = xml``; + } + + await mount(Root, fixture, { + templates: ` + + + + + `, + }); + expect(fixture.innerHTML).toBe(""); + }); }); diff --git a/tests/misc/__snapshots__/portal.test.ts.snap b/tests/misc/__snapshots__/portal.test.ts.snap index 19b2f544..a920d8da 100644 --- a/tests/misc/__snapshots__/portal.test.ts.snap +++ b/tests/misc/__snapshots__/portal.test.ts.snap @@ -646,7 +646,7 @@ exports[`Portal portal destroys on crash 2`] = ` let block1 = createBlock(\`\`); return function template(ctx, node, key = \\"\\") { - let txt1 = ctx['props'].error&&this.will.crash; + let txt1 = ctx['props'].error&&ctx['this'].will.crash; return block1([txt1]); } }"