From 6012d7638849ab23d0438d49c1a7fe904874c51a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Wed, 30 Jan 2019 14:56:47 +0100 Subject: [PATCH] fix issue with vnodes being patched twice --- web/static/src/ts/core/Widget.ts | 6 +++--- web/static/src/ts/core/qweb_vdom.ts | 16 +++++++--------- web/static/src/ts/widgets/Discuss.ts | 18 +++++++++++++++--- web/static/tests/core/widget.test.ts | 24 ++++++++++++++++++++++++ 4 files changed, 49 insertions(+), 15 deletions(-) diff --git a/web/static/src/ts/core/Widget.ts b/web/static/src/ts/core/Widget.ts index 055e1f23..2c4fa6d9 100644 --- a/web/static/src/ts/core/Widget.ts +++ b/web/static/src/ts/core/Widget.ts @@ -55,7 +55,6 @@ export class Widget { constructor(parent: Widget | T, props?: Props) { wl.push(this); - // is this a good idea? // Pro: if props is empty, we can create easily a widget // Con: this is not really safe @@ -211,14 +210,15 @@ export class Widget { /** * Only called by qweb t-widget directive */ - _mount(vnode: VNode) { - this.__widget__.vnode = vnode; + _mount(vnode: VNode, elm: HTMLElement): VNode { + this.__widget__.vnode = patch(elm, vnode); if (this.__widget__.parent) { if (this.__widget__.parent.__widget__.isMounted) { this.__widget__.isMounted = true; this.mounted(); } } + return this.__widget__.vnode; } private visitSubTree(callback: (w: Widget) => void) { diff --git a/web/static/src/ts/core/qweb_vdom.ts b/web/static/src/ts/core/qweb_vdom.ts index d282ccd3..f029a3e8 100644 --- a/web/static/src/ts/core/qweb_vdom.ts +++ b/web/static/src/ts/core/qweb_vdom.ts @@ -210,14 +210,14 @@ export class QWeb { const mainNode = doc.firstChild!; this._compileNode(mainNode, 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.shouldProtectContext) { - ctx.code.unshift(" context = Object.create(context);"); - } if (!ctx.rootNode) { throw new Error("A template should have one root node"); @@ -728,7 +728,6 @@ const widgetDirective: Directive = { let dummyID = ctx.generateID(); let defID = ctx.generateID(); let widgetID = ctx.generateID(); - ctx.addLine(`let _${dummyID} = {}; // DUMMY`); let keyID = key && ctx.generateID(); if (key) { // we bind a variable to the key (could be a complex expression, so we @@ -736,7 +735,7 @@ const widgetDirective: Directive = { ctx.addLine(`let key${keyID} = ${key};`); } ctx.addLine(`let _${dummyID}_index = c${ctx.parentNode}.length;`); - ctx.addLine(`c${ctx.parentNode}.push(_${dummyID});`); + ctx.addLine(`c${ctx.parentNode}.push(null);`); ctx.addLine(`let def${defID};`); let templateID = key ? `key${keyID}` @@ -746,11 +745,10 @@ const widgetDirective: Directive = { ctx.addLine( `let w${widgetID} = ${templateID} in context.__widget__.cmap ? context.__widget__.children[context.__widget__.cmap[${templateID}]] : false;` ); - ctx.addLine(`if (w${widgetID}) {`); ctx.indent(); ctx.addLine( - `def${defID} = w${widgetID}.updateProps(${props}).then(()=>{vnode=w${widgetID}.__widget__.vnode;c${ + `def${defID} = w${widgetID}.updateProps(${props}).then(()=>{let vnode=h(w${widgetID}.__widget__.vnode.sel, {key: ${templateID}});c${ ctx.parentNode }[_${dummyID}_index]=vnode;vnode.data.hook = {remove(){w${widgetID}.destroy()}}});` ); @@ -765,9 +763,9 @@ const widgetDirective: Directive = { `context.__widget__.cmap[${templateID}] = _${widgetID}.__widget__.id;` ); ctx.addLine( - `def${defID} = _${widgetID}._start().then(() => _${widgetID}._render()).then(vnode=>{c${ + `def${defID} = _${widgetID}._start().then(() => _${widgetID}._render()).then(vnode=>{let pvnode=h(vnode.sel, {key: ${templateID}});c${ ctx.parentNode - }[_${dummyID}_index]=vnode;vnode.data.hook = {create(_,vn){_${widgetID}._mount(vn)},remove(){_${widgetID}.destroy()}}});` + }[_${dummyID}_index]=pvnode;pvnode.data.hook = {insert(vn){let nvn=_${widgetID}._mount(vnode, vn.elm);pvnode.elm=nvn.elm},remove(){_${widgetID}.destroy()}}});` ); let ref = node.getAttribute("t-ref"); diff --git a/web/static/src/ts/widgets/Discuss.ts b/web/static/src/ts/widgets/Discuss.ts index 6e9c9fbf..69ed1544 100644 --- a/web/static/src/ts/widgets/Discuss.ts +++ b/web/static/src/ts/widgets/Discuss.ts @@ -22,16 +22,28 @@ const template = ` - - + + + `; +let n = 1; +class ChildWidget extends Widget { + name = "c"; + template = ``; + constructor(parent) { + super(parent); + this.state = { n }; + n++; + } +} + export class Discuss extends Widget { name = "discuss"; template = template; - widgets = { Clock, Counter, ColorWidget }; + widgets = { Clock, Counter, ColorWidget, ChildWidget }; state = { validcounter: true, color: "red", test: [1, 2, 3] }; mounted() {} diff --git a/web/static/tests/core/widget.test.ts b/web/static/tests/core/widget.test.ts index 0a54d132..3877eb00 100644 --- a/web/static/tests/core/widget.test.ts +++ b/web/static/tests/core/widget.test.ts @@ -714,3 +714,27 @@ describe("random stuff", () => { expect(fixture.innerHTML).toBe("
txttxt
"); }); }); + +describe("miscellaneous", () => { + test("updating widget immediately", async () => { + // in this situation, we protect against a bug that occurred: because of the + // interplay between widgets and vnodes, a sub widget vnode was patched + // twice. + class Parent extends Widget { + name = "a"; + template = `
`; + widgets = { child: Child }; + state = { flag: false }; + } + + class Child extends Widget { + template = `abcdef`; + } + + const widget = new Parent(env); + await widget.mount(fixture); + expect(fixture.innerHTML).toBe("
abc
"); + await widget.updateState({ flag: true }); + expect(fixture.innerHTML).toBe("
abcdef
"); + }); +});