From 8e1aa714362a56e76bfffc4cc38af815332467ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Fri, 5 Jul 2019 09:45:55 +0200 Subject: [PATCH] [FIX] component: scope issue with slots Slot templates need to be able to access variables from the parent scope. closes #228 --- src/component.ts | 24 +-- src/qweb_core.ts | 83 +++++++--- src/qweb_directives.ts | 10 +- src/qweb_extensions.ts | 24 ++- tests/__snapshots__/component.test.ts.snap | 173 ++++++++++++++++++++- tests/component.test.ts | 76 +++++++++ 6 files changed, 343 insertions(+), 47 deletions(-) diff --git a/src/component.ts b/src/component.ts index 98147c68..b1fa315f 100644 --- a/src/component.ts +++ b/src/component.ts @@ -274,7 +274,7 @@ export class Component { } } - async render(force: boolean = false, patchQueue?: any[]): Promise { + async render(force: boolean = false, patchQueue?: any[], scope?: any, vars?: any): Promise { const __owl__ = this.__owl__; if (!__owl__.isMounted) { return; @@ -283,7 +283,7 @@ export class Component { if (shouldPatch) { patchQueue = []; } - const renderVDom = this.__render(force, patchQueue); + const renderVDom = this.__render(force, patchQueue, scope, vars); const renderId = __owl__.renderId; await renderVDom; @@ -417,7 +417,9 @@ export class Component { async __updateProps( nextProps: Props, forceUpdate: boolean = false, - patchQueue?: any[] + patchQueue?: any[], + scope?: any, + vars?: any, ): Promise { const shouldUpdate = forceUpdate || this.shouldUpdate(nextProps); if (shouldUpdate) { @@ -427,7 +429,7 @@ export class Component { } await this.willUpdateProps(nextProps); this.props = nextProps; - await this.render(forceUpdate, patchQueue); + await this.render(forceUpdate, patchQueue, scope, vars); } } @@ -441,14 +443,14 @@ export class Component { __owl__.vnode = patch(target, vnode); } - __prepare(): Promise { + __prepare(scope?: Object, vars?: any): Promise { const __owl__ = this.__owl__; __owl__.renderProps = this.props; - __owl__.renderPromise = this.__prepareAndRender(); + __owl__.renderPromise = this.__prepareAndRender(scope, vars); return __owl__.renderPromise; } - async __prepareAndRender(): Promise { + async __prepareAndRender(scope?: Object, vars?: any): Promise { await this.willStart(); const __owl__ = this.__owl__; if (__owl__.isDestroyed) { @@ -480,10 +482,10 @@ export class Component { } __owl__.render = qweb.render.bind(qweb, this.template); this.__observeState(); - return this.__render(); + return this.__render(false, [], scope, vars); } - async __render(force: boolean = false, patchQueue: any[] = []): Promise { + async __render(force: boolean = false, patchQueue: any[] = [], scope?: Object, vars?: any): Promise { const __owl__ = this.__owl__; __owl__.renderId++; const promises: Promise[] = []; @@ -499,7 +501,9 @@ export class Component { handlers: __owl__.boundHandlers, mountedHandlers: __owl__.mountedHandlers, forceUpdate: force, - patchQueue + patchQueue, + scope, + vars }); patch.push(vnode); if (__owl__.observer) { diff --git a/src/qweb_core.ts b/src/qweb_core.ts index 55a8b632..e7bf6c24 100644 --- a/src/qweb_core.ts +++ b/src/qweb_core.ts @@ -288,45 +288,44 @@ export class QWeb extends EventBus { return template.fn.call(this, context, extra); } - _compile(name: string, elem: Element, parentNode?: number): CompiledTemplate { + _compile(name: string, elem: Element, parentContext?: Context): CompiledTemplate { const isDebug = elem.attributes.hasOwnProperty("t-debug"); const ctx = new Context(name); - if (parentNode) { - ctx.nextID = parentNode + 1; - ctx.parentNode = parentNode; + if (parentContext) { + ctx.variables = Object.create(parentContext.variables); + ctx.nextID = parentContext.parentNode! + 1; + ctx.parentNode = parentContext.parentNode!; ctx.allowMultipleRoots = true; - ctx.addLine(`let c${parentNode} = extra.parentNode;`); + ctx.addLine(`let c${ctx.parentNode} = extra.parentNode;`); + + for (let v in parentContext.variables) { + let variable = parentContext.variables[v]; + if (variable.id) { + ctx.addLine(`let ${variable.id} = extra.vars.${variable.id}`); + } + } + } + if (parentContext) { + ctx.addLine(" Object.assign(context, extra.scope);"); } this._compileNode(elem, ctx); - if (ctx.shouldProtectContext) { - ctx.code.unshift(" context = Object.create(context);"); - } - if (ctx.shouldDefineOwner) { - // this is necessary to prevent some directives (t-forach for ex) to - // pollute the rendering context by adding some keys in it. - ctx.code.unshift(" let owner = context;"); - } - if (ctx.shouldDefineQWeb) { - ctx.code.unshift(" let QWeb = this.constructor;"); - } - if (ctx.shouldDefineUtils) { - ctx.code.unshift(" let utils = this.utils;"); - } - - if (!parentNode) { + if (!parentContext) { if (!ctx.rootNode) { throw new Error("A template should have one root node"); } ctx.addLine(`return vn${ctx.rootNode};`); } + + let code = ctx.generateCode(); + let template; try { - template = new Function("context", "extra", ctx.code.join("\n")) as CompiledTemplate; + template = new Function("context", "extra", code.join("\n")) as CompiledTemplate; } catch (e) { const templateName = ctx.templateName.replace(/`/g, "'"); console.groupCollapsed(`Invalid Code generated by ${templateName}`); - console.warn(ctx.code.join("\n")); + console.warn(code.join("\n")); console.groupEnd(); throw new Error( `Invalid generated code while compiling template '${templateName}': ${e.message}` @@ -672,10 +671,12 @@ export class Context { shouldDefineQWeb: boolean = false; shouldDefineUtils: boolean = false; shouldProtectContext: boolean = false; + shouldTrackScope: boolean = false; inLoop: boolean = false; inPreTag: boolean = false; templateName: string; allowMultipleRoots: boolean = false; + scopeVars: any[] = []; constructor(name?: string) { this.rootContext = this; @@ -688,6 +689,34 @@ export class Context { return id; } + generateCode(): string[] { + const shouldTrackScope = this.shouldTrackScope && this.scopeVars.length; + if (shouldTrackScope) { + // add some vars to scope if needed + for (let scopeVar of this.scopeVars.reverse()) { + let { index, key, indent } = scopeVar; + const prefix = new Array(indent + 2).join(" "); + this.code.splice(index + 1, 0, prefix + `scope.${key} = context.${key};`); + } + this.code.unshift(" const scope = Object.create(null);"); + } + if (this.shouldProtectContext) { + this.code.unshift(" context = Object.create(context);"); + } + if (this.shouldDefineOwner) { + // this is necessary to prevent some directives (t-forach for ex) to + // pollute the rendering context by adding some keys in it. + this.code.unshift(" let owner = context;"); + } + if (this.shouldDefineQWeb) { + this.code.unshift(" let QWeb = this.constructor;"); + } + if (this.shouldDefineUtils) { + this.code.unshift(" let utils = this.utils;"); + } + return this.code; + } + withParent(node: number): Context { if ( !this.allowMultipleRoots && @@ -716,9 +745,15 @@ export class Context { this.indentLevel--; } - addLine(line: string) { + addLine(line: string): number { const prefix = new Array(this.indentLevel + 2).join(" "); this.code.push(prefix + line); + return this.code.length - 1; + } + + addToScope(key: string, expr: string) { + const index = this.addLine(`context.${key} = ${expr};`); + this.rootContext.scopeVars.push({ index, key, indent: this.indentLevel }); } addIf(condition: string) { diff --git a/src/qweb_directives.ts b/src/qweb_directives.ts index 0a49f63f..ad2ab4fb 100644 --- a/src/qweb_directives.ts +++ b/src/qweb_directives.ts @@ -255,11 +255,11 @@ QWeb.addDirective({ ctx.addLine(`var _length${keysID} = _${keysID}.length;`); ctx.addLine(`for (let i = 0; i < _length${keysID}; i++) {`); ctx.indent(); - ctx.addLine(`context.${name}_first = i === 0;`); - ctx.addLine(`context.${name}_last = i === _length${keysID} - 1;`); - ctx.addLine(`context.${name}_index = i;`); - ctx.addLine(`context.${name} = _${keysID}[i];`); - ctx.addLine(`context.${name}_value = _${valuesID}[i];`); + ctx.addToScope(name + '_first', 'i === 0'); + ctx.addToScope(name + '_last', `i === _length${keysID} - 1`); + ctx.addToScope(name + '_index', 'i'); + ctx.addToScope(name, `_${keysID}[i]`); + ctx.addToScope(name + '_value', `_${valuesID}[i]`); const nodeCopy = node.cloneNode(true); let shouldWarn = nodeCopy.tagName !== "t" && !nodeCopy.hasAttribute("t-key"); if (!shouldWarn && node.tagName === "t") { diff --git a/src/qweb_extensions.ts b/src/qweb_extensions.ts index 565be819..d962c617 100644 --- a/src/qweb_extensions.ts +++ b/src/qweb_extensions.ts @@ -556,7 +556,16 @@ QWeb.addDirective({ ctx.addLine(`context.__owl__.cmap[${templateID}] = w${componentID}.__owl__.id;`); // SLOTS - if (node.childNodes.length) { + const varDefs: string[] = []; + const hasSlots = node.childNodes.length; + if (hasSlots) { + ctx.rootContext.shouldTrackScope = true; + for (let v of Object.values(ctx.variables)) { + if (v["id"]) { + varDefs.push(v["id"]); + } + } + const clone = node.cloneNode(true); const slotNodes = clone.querySelectorAll("[t-set]"); const slotId = qweb.nextSlotId++; @@ -567,7 +576,7 @@ QWeb.addDirective({ slotNode.parentElement!.removeChild(slotNode); const key = slotNode.getAttribute("t-set")!; slotNode.removeAttribute("t-set"); - const slotFn = qweb._compile(`slot_${key}_template`, slotNode, ctx.parentNode!); + const slotFn = qweb._compile(`slot_${key}_template`, slotNode, ctx); qweb.slots[`${slotId}_${key}`] = slotFn.bind(qweb); } } @@ -576,12 +585,14 @@ QWeb.addDirective({ for (let child of Object.values(clone.childNodes)) { t.appendChild(child); } - const slotFn = qweb._compile(`slot_default_template`, t, ctx.parentNode!); + const slotFn = qweb._compile(`slot_default_template`, t, ctx); qweb.slots[`${slotId}_default`] = slotFn.bind(qweb); } } - ctx.addLine(`def${defID} = w${componentID}.__prepare();`); + const scopeVars = + hasSlots && ctx.scopeVars.length ? `Object.assign({}, scope), {${varDefs.join(",")}}` : ""; + ctx.addLine(`def${defID} = w${componentID}.__prepare(${scopeVars});`); // hack: specify empty remove hook to prevent the node from being removed from the DOM ctx.addLine( `def${defID} = def${defID}.then(vnode=>{${createHook}let pvnode=h(vnode.sel, {key: ${templateID}, hook: {insert(vn) {let nvn=w${componentID}.__mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;${refExpr}${transitionsInsertCode}},remove() {},destroy(vn) {${finalizeComponentCode}}}});c${ @@ -596,7 +607,8 @@ QWeb.addDirective({ ctx.addLine(`utils.validateProps(w${componentID}.constructor, props${componentID})`); } ctx.addLine( - `def${defID} = def${defID} || w${componentID}.__updateProps(props${componentID}, extra.forceUpdate, ${patchQueueCode});` + `def${defID} = def${defID} || w${componentID}.__updateProps(props${componentID}, extra.forceUpdate, ${patchQueueCode}${scopeVars && + ", " + scopeVars});` ); let keepAliveCode = ""; if (keepAlive) { @@ -774,7 +786,7 @@ QWeb.addDirective({ ctx.addLine( `slot${slotKey}(context.__owl__.parent, Object.assign({}, extra, {parentNode: c${ ctx.parentNode - }}));` + }, vars: extra.vars}));` ); ctx.closeIf(); return true; diff --git a/tests/__snapshots__/component.test.ts.snap b/tests/__snapshots__/component.test.ts.snap index aa8a5cd3..c4f6520d 100644 --- a/tests/__snapshots__/component.test.ts.snap +++ b/tests/__snapshots__/component.test.ts.snap @@ -1193,14 +1193,183 @@ exports[`t-slot directive can define and call slots 2`] = ` c1.push(vn2); const slot3 = this.slots[context.__owl__.slotId + '_' + 'header']; if (slot3) { - slot3(context.__owl__.parent, Object.assign({}, extra, {parentNode: c2})); + slot3(context.__owl__.parent, Object.assign({}, extra, {parentNode: c2, vars: extra.vars})); } let c4 = [], p4 = {key:4}; var vn4 = h('div', p4, c4); c1.push(vn4); const slot5 = this.slots[context.__owl__.slotId + '_' + 'footer']; if (slot5) { - slot5(context.__owl__.parent, Object.assign({}, extra, {parentNode: c4})); + slot5(context.__owl__.parent, Object.assign({}, extra, {parentNode: c4, vars: extra.vars})); + } + return vn1; +}" +`; + +exports[`t-slot directive slots are rendered with proper context, part 2 1`] = ` +"function anonymous(context,extra +) { + var h = this.utils.h; + var _1 = context['props'].to; + let c2 = [], p2 = {key:2,attrs:{href: _1}}; + var vn2 = h('a', p2, c2); + const slot3 = this.slots[context.__owl__.slotId + '_' + 'default']; + if (slot3) { + slot3(context.__owl__.parent, Object.assign({}, extra, {parentNode: c2, vars: extra.vars})); + } + return vn2; +}" +`; + +exports[`t-slot directive slots are rendered with proper context, part 2 2`] = ` +"function anonymous(context,extra +) { + let utils = this.utils; + let QWeb = this.constructor; + let owner = context; + context = Object.create(context); + const scope = Object.create(null); + var h = this.utils.h; + let c1 = [], p1 = {key:1}; + var vn1 = h('div', p1, c1); + let c2 = [], p2 = {key:2}; + var vn2 = h('u', p2, c2); + c1.push(vn2); + var _3 = context['state'].users; + if (!_3) { throw new Error('QWeb error: Invalid loop expression')} + var _4 = _5 = _3; + if (!(_3 instanceof Array)) { + _4 = Object.keys(_3); + _5 = Object.values(_3); + } + var _length4 = _4.length; + for (let i = 0; i < _length4; i++) { + context.user_first = i === 0; + scope.user_first = context.user_first; + context.user_last = i === _length4 - 1; + scope.user_last = context.user_last; + context.user_index = i; + scope.user_index = context.user_index; + context.user = _4[i]; + scope.user = context.user; + context.user_value = _5[i]; + scope.user_value = context.user_value; + let c6 = [], p6 = {key:context['user'].id}; + var vn6 = h('li', p6, c6); + c2.push(vn6); + //COMPONENT + let def8; + let w9 = String(-9 - i) in context.__owl__.cmap ? context.__owl__.children[context.__owl__.cmap[String(-9 - i)]] : false; + let _7_index = c6.length; + c6.push(null); + let props9 = {to:'/user/'+context['user'].id}; + if (w9 && w9.__owl__.renderPromise && !w9.__owl__.vnode) { + if (utils.shallowEqual(props9, w9.__owl__.renderProps)) { + def8 = w9.__owl__.renderPromise; + } else { + w9.destroy(); + w9 = false; + } + } + if (!w9) { + let componentKey9 = \`Link\`; + let W9 = context.components && context.components[componentKey9] || QWeb.components[componentKey9]; + if (!W9) {throw new Error('Cannot find the definition of component \\"' + componentKey9 + '\\"')} + w9 = new W9(owner, props9); + context.__owl__.cmap[String(-9 - i)] = w9.__owl__.id; + w9.__owl__.slotId = 1; + def8 = w9.__prepare(Object.assign({}, scope), {}); + def8 = def8.then(vnode=>{let pvnode=h(vnode.sel, {key: String(-9 - i), hook: {insert(vn) {let nvn=w9.__mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w9.destroy();}}});c6[_7_index]=pvnode;w9.__owl__.pvnode = pvnode;}); + } else { + def8 = def8 || w9.__updateProps(props9, extra.forceUpdate, extra.patchQueue, Object.assign({}, scope), {}); + def8 = def8.then(()=>{if (w9.__owl__.isDestroyed) {return};let pvnode=w9.__owl__.pvnode;c6[_7_index]=pvnode;}); + } + extra.promises.push(def8); + } + return vn1; +}" +`; + +exports[`t-slot directive slots are rendered with proper context, part 3 1`] = ` +"function anonymous(context,extra +) { + var h = this.utils.h; + var _1 = context['props'].to; + let c2 = [], p2 = {key:2,attrs:{href: _1}}; + var vn2 = h('a', p2, c2); + const slot3 = this.slots[context.__owl__.slotId + '_' + 'default']; + if (slot3) { + slot3(context.__owl__.parent, Object.assign({}, extra, {parentNode: c2, vars: extra.vars})); + } + return vn2; +}" +`; + +exports[`t-slot directive slots are rendered with proper context, part 3 2`] = ` +"function anonymous(context,extra +) { + let utils = this.utils; + let QWeb = this.constructor; + let owner = context; + context = Object.create(context); + const scope = Object.create(null); + var h = this.utils.h; + let c1 = [], p1 = {key:1}; + var vn1 = h('div', p1, c1); + let c2 = [], p2 = {key:2}; + var vn2 = h('u', p2, c2); + c1.push(vn2); + var _3 = context['state'].users; + if (!_3) { throw new Error('QWeb error: Invalid loop expression')} + var _4 = _5 = _3; + if (!(_3 instanceof Array)) { + _4 = Object.keys(_3); + _5 = Object.values(_3); + } + var _length4 = _4.length; + for (let i = 0; i < _length4; i++) { + context.user_first = i === 0; + scope.user_first = context.user_first; + context.user_last = i === _length4 - 1; + scope.user_last = context.user_last; + context.user_index = i; + scope.user_index = context.user_index; + context.user = _4[i]; + scope.user = context.user; + context.user_value = _5[i]; + scope.user_value = context.user_value; + let c6 = [], p6 = {key:context['user'].id}; + var vn6 = h('li', p6, c6); + c2.push(vn6); + var _7 = 'User '+context['user'].name; + //COMPONENT + let def9; + let w10 = String(-10 - i) in context.__owl__.cmap ? context.__owl__.children[context.__owl__.cmap[String(-10 - i)]] : false; + let _8_index = c6.length; + c6.push(null); + let props10 = {to:'/user/'+context['user'].id}; + if (w10 && w10.__owl__.renderPromise && !w10.__owl__.vnode) { + if (utils.shallowEqual(props10, w10.__owl__.renderProps)) { + def9 = w10.__owl__.renderPromise; + } else { + w10.destroy(); + w10 = false; + } + } + if (!w10) { + let componentKey10 = \`Link\`; + let W10 = context.components && context.components[componentKey10] || QWeb.components[componentKey10]; + if (!W10) {throw new Error('Cannot find the definition of component \\"' + componentKey10 + '\\"')} + w10 = new W10(owner, props10); + context.__owl__.cmap[String(-10 - i)] = w10.__owl__.id; + w10.__owl__.slotId = 1; + def9 = w10.__prepare(Object.assign({}, scope), {_7}); + def9 = def9.then(vnode=>{let pvnode=h(vnode.sel, {key: String(-10 - i), hook: {insert(vn) {let nvn=w10.__mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w10.destroy();}}});c6[_8_index]=pvnode;w10.__owl__.pvnode = pvnode;}); + } else { + def9 = def9 || w10.__updateProps(props10, extra.forceUpdate, extra.patchQueue, Object.assign({}, scope), {_7}); + def9 = def9.then(()=>{if (w10.__owl__.isDestroyed) {return};let pvnode=w10.__owl__.pvnode;c6[_8_index]=pvnode;}); + } + extra.promises.push(def9); } return vn1; }" diff --git a/tests/component.test.ts b/tests/component.test.ts index bbe15910..38b26df5 100644 --- a/tests/component.test.ts +++ b/tests/component.test.ts @@ -2920,6 +2920,82 @@ describe("t-slot directive", () => { ); }); + test("slots are rendered with proper context, part 2", async () => { + env.qweb.addTemplates(` + + + + +
+
  • + User +
    • +
    +     + `); + class Link extends Widget {} + + class App extends Widget { + state = {users: [{id: 1, name: 'Aaron'}, {id: 2, name: 'David'}]}; + components = { Link }; + } + + const app = new App(env); + await app.mount(fixture); + + expect(fixture.innerHTML).toBe( + "" + ); + expect(env.qweb.templates.Link.fn.toString()).toMatchSnapshot(); + expect(env.qweb.templates.App.fn.toString()).toMatchSnapshot(); + + // test updateprops here + app.state.users[1].name = "Mathieu"; + await nextTick(); + expect(fixture.innerHTML).toBe( + "" + ); + }); + + test("slots are rendered with proper context, part 3", async () => { + env.qweb.addTemplates(` + + + + +
    +
  • + + +
    • +
    +     + `); + class Link extends Widget {} + + class App extends Widget { + state = {users: [{id: 1, name: 'Aaron'}, {id: 2, name: 'David'}]}; + components = { Link }; + } + + const app = new App(env); + await app.mount(fixture); + + expect(fixture.innerHTML).toBe( + "" + ); + expect(env.qweb.templates.Link.fn.toString()).toMatchSnapshot(); + expect(env.qweb.templates.App.fn.toString()).toMatchSnapshot(); + + // test updateprops here + app.state.users[1].name = "Mathieu"; + await nextTick(); + expect(fixture.innerHTML).toBe( + "" + ); + + }); + test("refs are properly bound in slots", async () => { env.qweb.addTemplates(`