From cfdce29ce757e401ec08aee43abc53761d7d21b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Tue, 3 Sep 2019 09:32:38 +0200 Subject: [PATCH] [FIX] component: avoid key collisions in sub components This is a difficult part of owl: we need to be able to reconcile the vdom generated (this is done with our virtual dom algorithm), but also to be able to reconcile the proper components. The issue here is that when we are in a list, with a t-key attribute on all list nodes, and those list nodes contains sub component, then the component system used the index of the list, and did not take into account the key from the parent. The fix is to keep track of the current key in the context, and uses that as a part of the templateId. --- src/component/directive.ts | 18 ++++---- src/qweb/base_directives.ts | 2 + src/qweb/context.ts | 2 + src/qweb/qweb.ts | 5 ++- .../__snapshots__/component.test.ts.snap | 43 +++++++++++-------- tests/component/component.test.ts | 31 +++++++++++++ tests/qweb/__snapshots__/qweb.test.ts.snap | 18 +++++--- 7 files changed, 85 insertions(+), 34 deletions(-) diff --git a/src/component/directive.ts b/src/component/directive.ts index 21bb9c35..73cc5eb7 100644 --- a/src/component/directive.ts +++ b/src/component/directive.ts @@ -244,17 +244,17 @@ QWeb.addDirective({ let templateID = key ? `key${keyID}` : ctx.inLoop - ? `String(-${componentID} - i)` + ? ctx.currentKey + ? `String(${ctx.currentKey} + '_k_' + i)` + : `String(-${componentID} - i)` : String(componentID); if (ctx.allowMultipleRoots) { - // necessary to prevent collisions - if (!key && ctx.inLoop) { - let id = ctx.generateID(); - ctx.addLine(`let template${id} = "_slot_" + String(-${componentID} - i)`); - templateID = `template${id}`; - } else { - templateID = `"_slot_${templateID}"`; - } + templateID = `"_slot_${templateID}"`; + } + if (key || ctx.inLoop) { + let id = ctx.generateID(); + ctx.addLine(`let templateId${id} = ${templateID};`); + templateID = `templateId${id}`; } let ref = node.getAttribute("t-ref"); diff --git a/src/qweb/base_directives.ts b/src/qweb/base_directives.ts index 74817be0..90a9b258 100644 --- a/src/qweb/base_directives.ts +++ b/src/qweb/base_directives.ts @@ -82,6 +82,7 @@ QWeb.addDirective({ if (node.nodeName !== "t") { let nodeID = qweb._compileGenericNode(node, ctx); ctx = ctx.withParent(nodeID); + ctx = ctx.subContext("currentKey", ctx.lastNodeKey); } let value = ctx.getValue(node.getAttribute("t-esc")!); compileValueNode(value, node, qweb, ctx.subContext("escaping", true)); @@ -96,6 +97,7 @@ QWeb.addDirective({ if (node.nodeName !== "t") { let nodeID = qweb._compileGenericNode(node, ctx); ctx = ctx.withParent(nodeID); + ctx = ctx.subContext("currentKey", ctx.lastNodeKey); } let value = ctx.getValue(node.getAttribute("t-raw")!); compileValueNode(value, node, qweb, ctx); diff --git a/src/qweb/context.ts b/src/qweb/context.ts index 21ced242..94d61ab7 100644 --- a/src/qweb/context.ts +++ b/src/qweb/context.ts @@ -28,6 +28,8 @@ export class Context { allowMultipleRoots: boolean = false; hasParentWidget: boolean = false; scopeVars: any[] = []; + currentKey: string = ""; + lastNodeKey: string = ""; // temp variable to communicate to previous caller constructor(name?: string) { this.rootContext = this; diff --git a/src/qweb/qweb.ts b/src/qweb/qweb.ts index 968b4be1..3cf8e5b3 100644 --- a/src/qweb/qweb.ts +++ b/src/qweb/qweb.ts @@ -473,6 +473,7 @@ export class QWeb extends EventBus { if (node.nodeName !== "t") { let nodeID = this._compileGenericNode(node, ctx, withHandlers); ctx = ctx.withParent(nodeID); + ctx = ctx.subContext("currentKey", ctx.lastNodeKey); let nodeHooks = {}; let addNodeHook = function(hook, handler) { nodeHooks[hook] = nodeHooks[hook] || []; @@ -641,7 +642,9 @@ export class QWeb extends EventBus { let nodeID = ctx.generateID(); let nodeKey: any = (node).getAttribute("t-key"); if (nodeKey) { - nodeKey = ctx.formatExpression(nodeKey); + ctx.addLine(`const nodeKey${nodeID} = ${ctx.formatExpression(nodeKey)}`); + nodeKey = `nodeKey${nodeID}`; + ctx.lastNodeKey = nodeKey; } else { nodeKey = nodeID; } diff --git a/tests/component/__snapshots__/component.test.ts.snap b/tests/component/__snapshots__/component.test.ts.snap index 9b891433..38ad8287 100644 --- a/tests/component/__snapshots__/component.test.ts.snap +++ b/tests/component/__snapshots__/component.test.ts.snap @@ -518,7 +518,8 @@ exports[`composition sub components with some state rendered in a loop 1`] = ` //COMPONENT let key8 = 'key' + context['number']; let def6; - let w7 = key8 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[key8]] : false; + let templateId9 = key8; + let w7 = templateId9 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[templateId9]] : false; let _5_index = c1.length; c1.push(null); let props7 = {}; @@ -535,9 +536,9 @@ exports[`composition sub components with some state rendered in a loop 1`] = ` let W7 = context.components && context.components[componentKey7] || QWeb.components[componentKey7]; if (!W7) {throw new Error('Cannot find the definition of component \\"' + componentKey7 + '\\"')} w7 = new W7(parent, props7); - parent.__owl__.cmap[key8] = w7.__owl__.id; + parent.__owl__.cmap[templateId9] = w7.__owl__.id; def6 = w7.__prepare(); - def6 = def6.then(vnode=>{if (w7.__owl__.isDestroyed){return}let pvnode=h(vnode.sel, {key: key8, hook: {insert(vn) {let nvn=w7.__mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w7.destroy();}}});c1[_5_index]=pvnode;w7.__owl__.pvnode = pvnode;}); + def6 = def6.then(vnode=>{if (w7.__owl__.isDestroyed){return}let pvnode=h(vnode.sel, {key: templateId9, hook: {insert(vn) {let nvn=w7.__mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w7.destroy();}}});c1[_5_index]=pvnode;w7.__owl__.pvnode = pvnode;}); } else { def6 = def6 || w7.__updateProps(props7, extra.forceUpdate, extra.patchQueue); def6 = def6.then(()=>{if (w7.__owl__.isDestroyed) {return};let pvnode=w7.__owl__.pvnode;c1[_5_index]=pvnode;}); @@ -1014,7 +1015,8 @@ exports[`random stuff/miscellaneous snapshotting compiled code 1`] = ` //COMPONENT let key5 = 'key' + 'somestring'; let def3; - let w4 = key5 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[key5]] : false; + let templateId6 = key5; + let w4 = templateId6 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[templateId6]] : false; let _2_index = c1.length; c1.push(null); let props4 = {flag:context['state'].flag}; @@ -1031,9 +1033,9 @@ exports[`random stuff/miscellaneous snapshotting compiled code 1`] = ` let W4 = context.components && context.components[componentKey4] || QWeb.components[componentKey4]; if (!W4) {throw new Error('Cannot find the definition of component \\"' + componentKey4 + '\\"')} w4 = new W4(parent, props4); - parent.__owl__.cmap[key5] = w4.__owl__.id; + parent.__owl__.cmap[templateId6] = w4.__owl__.id; def3 = w4.__prepare(); - def3 = def3.then(vnode=>{if (w4.__owl__.isDestroyed){return}let pvnode=h(vnode.sel, {key: key5, hook: {insert(vn) {let nvn=w4.__mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w4.destroy();}}});c1[_2_index]=pvnode;w4.__owl__.pvnode = pvnode;}); + def3 = def3.then(vnode=>{if (w4.__owl__.isDestroyed){return}let pvnode=h(vnode.sel, {key: templateId6, hook: {insert(vn) {let nvn=w4.__mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w4.destroy();}}});c1[_2_index]=pvnode;w4.__owl__.pvnode = pvnode;}); } else { def3 = def3 || w4.__updateProps(props4, extra.forceUpdate, extra.patchQueue); def3 = def3.then(()=>{if (w4.__owl__.isDestroyed) {return};let pvnode=w4.__owl__.pvnode;c1[_2_index]=pvnode;}); @@ -1071,8 +1073,9 @@ exports[`random stuff/miscellaneous t-on with handler bound to dynamic argument //COMPONENT let key8 = 'key' + context['item']; let def6; - let arg9 = context['item']; - let w7 = key8 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[key8]] : false; + let templateId9 = key8; + let arg10 = context['item']; + let w7 = templateId9 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[templateId9]] : false; let _5_index = c1.length; c1.push(null); let props7 = {}; @@ -1089,9 +1092,9 @@ exports[`random stuff/miscellaneous t-on with handler bound to dynamic argument let W7 = context.components && context.components[componentKey7] || QWeb.components[componentKey7]; if (!W7) {throw new Error('Cannot find the definition of component \\"' + componentKey7 + '\\"')} w7 = new W7(parent, props7); - parent.__owl__.cmap[key8] = w7.__owl__.id; + parent.__owl__.cmap[templateId9] = w7.__owl__.id; def6 = w7.__prepare(); - def6 = def6.then(vnode=>{if (w7.__owl__.isDestroyed){return}vnode.data.hook = {create(_, vn){vn.elm.addEventListener('ev', owner['onEv'].bind(owner, arg9));}};let pvnode=h(vnode.sel, {key: key8, hook: {insert(vn) {let nvn=w7.__mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w7.destroy();}}});c1[_5_index]=pvnode;w7.__owl__.pvnode = pvnode;}); + def6 = def6.then(vnode=>{if (w7.__owl__.isDestroyed){return}vnode.data.hook = {create(_, vn){vn.elm.addEventListener('ev', owner['onEv'].bind(owner, arg10));}};let pvnode=h(vnode.sel, {key: templateId9, hook: {insert(vn) {let nvn=w7.__mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w7.destroy();}}});c1[_5_index]=pvnode;w7.__owl__.pvnode = pvnode;}); } else { def6 = def6 || w7.__updateProps(props7, extra.forceUpdate, extra.patchQueue); def6 = def6.then(()=>{if (w7.__owl__.isDestroyed) {return};let pvnode=w7.__owl__.pvnode;c1[_5_index]=pvnode;}); @@ -1366,12 +1369,14 @@ exports[`t-slot directive slots are rendered with proper context, part 2 2`] = ` scope.user = context.user; context.user_value = _5[i]; scope.user_value = context.user_value; - let c6 = [], p6 = {key:context['user'].id}; + const nodeKey6 = context['user'].id + let c6 = [], p6 = {key:nodeKey6}; var vn6 = h('li', p6, c6); c2.push(vn6); //COMPONENT let def8; - let w9 = String(-9 - i) in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[String(-9 - i)]] : false; + let templateId10 = String(nodeKey6 + '_k_' + i); + let w9 = templateId10 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[templateId10]] : false; let _7_index = c6.length; c6.push(null); let props9 = {to:'/user/'+context['user'].id}; @@ -1388,10 +1393,10 @@ exports[`t-slot directive slots are rendered with proper context, part 2 2`] = ` 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(parent, props9); - parent.__owl__.cmap[String(-9 - i)] = w9.__owl__.id; + parent.__owl__.cmap[templateId10] = w9.__owl__.id; w9.__owl__.slotId = 1; def8 = w9.__prepare(Object.assign({}, scope)); - def8 = def8.then(vnode=>{if (w9.__owl__.isDestroyed){return}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;}); + def8 = def8.then(vnode=>{if (w9.__owl__.isDestroyed){return}let pvnode=h(vnode.sel, {key: templateId10, 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;}); @@ -1452,13 +1457,15 @@ exports[`t-slot directive slots are rendered with proper context, part 3 2`] = ` scope.user = context.user; context.user_value = _5[i]; scope.user_value = context.user_value; - let c6 = [], p6 = {key:context['user'].id}; + const nodeKey6 = context['user'].id + let c6 = [], p6 = {key:nodeKey6}; var vn6 = h('li', p6, c6); c2.push(vn6); var _7 = 'User '+context['user'].name; //COMPONENT let def9; - let w10 = String(-10 - i) in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[String(-10 - i)]] : false; + let templateId11 = String(nodeKey6 + '_k_' + i); + let w10 = templateId11 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[templateId11]] : false; let _8_index = c6.length; c6.push(null); let props10 = {to:'/user/'+context['user'].id}; @@ -1475,10 +1482,10 @@ exports[`t-slot directive slots are rendered with proper context, part 3 2`] = ` 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(parent, props10); - parent.__owl__.cmap[String(-10 - i)] = w10.__owl__.id; + parent.__owl__.cmap[templateId11] = w10.__owl__.id; w10.__owl__.slotId = 1; def9 = w10.__prepare(Object.assign({}, scope), {_7}); - def9 = def9.then(vnode=>{if (w10.__owl__.isDestroyed){return}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;}); + def9 = def9.then(vnode=>{if (w10.__owl__.isDestroyed){return}let pvnode=h(vnode.sel, {key: templateId11, 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;}); diff --git a/tests/component/component.test.ts b/tests/component/component.test.ts index 19586930..56522a96 100644 --- a/tests/component/component.test.ts +++ b/tests/component/component.test.ts @@ -1342,6 +1342,37 @@ describe("composition", () => { ); }); + test("list of sub components inside other nodes", async () => { + // this confuses the patching algorithm... + env.qweb.addTemplate("ChildWidget", `child`); + + env.qweb.addTemplates(` + +
+
+ +
+
+ asdf + `); + + class SubWidget extends Widget {} + class Parent extends Widget { + components = { SubWidget }; + state = { blips: [{ a: "a", id: 1 }, { b: "b", id: 2 }, { c: "c", id: 4 }] }; + } + const parent = new Parent(env); + await parent.mount(fixture); + expect(fixture.innerHTML).toBe( + "
asdf
asdf
asdf
" + ); + parent.state.blips.splice(0, 1); + await nextTick(); + expect(fixture.innerHTML).toBe( + "
asdf
asdf
" + ); + }); + test("t-component with dynamic value", async () => { env.qweb.addTemplate("ParentWidget", `
`); class ParentWidget extends Widget { diff --git a/tests/qweb/__snapshots__/qweb.test.ts.snap b/tests/qweb/__snapshots__/qweb.test.ts.snap index 136629be..7a11fdfe 100644 --- a/tests/qweb/__snapshots__/qweb.test.ts.snap +++ b/tests/qweb/__snapshots__/qweb.test.ts.snap @@ -361,7 +361,8 @@ exports[`foreach iterate on items (on a element node) 1`] = ` context.item_index = i; context.item = _3[i]; context.item_value = _4[i]; - let c5 = [], p5 = {key:context['item']}; + const nodeKey5 = context['item'] + let c5 = [], p5 = {key:nodeKey5}; var vn5 = h('span', p5, c5); c1.push(vn5); var _6 = context['item']; @@ -1095,7 +1096,8 @@ exports[`t-key can use t-key directive on a node 1`] = ` "function anonymous(context,extra ) { var h = this.h; - let c1 = [], p1 = {key:context['beer'].id}; + const nodeKey1 = context['beer'].id + let c1 = [], p1 = {key:nodeKey1}; var vn1 = h('div', p1, c1); var _2 = context['beer'].name; if (_2 || _2 === 0) { @@ -1126,7 +1128,8 @@ exports[`t-key t-key directive in a list 1`] = ` context.beer_index = i; context.beer = _3[i]; context.beer_value = _4[i]; - let c5 = [], p5 = {key:context['beer'].id}; + const nodeKey5 = context['beer'].id + let c5 = [], p5 = {key:nodeKey5}; var vn5 = h('li', p5, c5); c1.push(vn5); var _6 = context['beer'].name; @@ -1227,7 +1230,8 @@ exports[`t-on can bind handlers with loop variable as argument 1`] = ` context.action_index = i; context.action = _3[i]; context.action_value = _4[i]; - let c5 = [], p5 = {key:context['action_index']}; + const nodeKey5 = context['action_index'] + let c5 = [], p5 = {key:nodeKey5}; var vn5 = h('li', p5, c5); c1.push(vn5); let c6 = [], p6 = {key:6,on:{}}; @@ -1560,7 +1564,8 @@ exports[`t-ref refs in a loop 1`] = ` context.item_index = i; context.item = _3[i]; context.item_value = _4[i]; - let c5 = [], p5 = {key:context['item']}; + const nodeKey5 = context['item'] + let c5 = [], p5 = {key:nodeKey5}; var vn5 = h('div', p5, c5); c1.push(vn5); const ref6 = (context['item']); @@ -1720,7 +1725,8 @@ exports[`t-set t-set should reuse variable if possible 1`] = ` context.elem_index = i; context.elem = _4[i]; context.elem_value = _5[i]; - let c6 = [], p6 = {key:context['elem_index']}; + const nodeKey6 = context['elem_index'] + let c6 = [], p6 = {key:nodeKey6}; var vn6 = h('div', p6, c6); c1.push(vn6); let c7 = [], p7 = {key:7};