diff --git a/src/component/component.ts b/src/component/component.ts index 84a8dd3b..bd2bcd2a 100644 --- a/src/component/component.ts +++ b/src/component/component.ts @@ -656,9 +656,28 @@ export class Component { // destroyed right now, because they are not in the DOM, and thus we won't // be notified later on (when patching), that they are removed from the DOM for (let childKey in __owl__.children) { - let child = __owl__.children[childKey]; - if (!child.__owl__.isMounted && child.__owl__.parentLastFiberId < fiber.id) { - child.destroy(); + const child = __owl__.children[childKey]; + const childOwl = child.__owl__; + if (!childOwl.isMounted && childOwl.parentLastFiberId < fiber.id) { + // we only do here a "soft" destroy, meaning that we leave the child + // dom node alone, without removing it. Most of the time, it does not + // matter, because the child component is already unmounted. However, + // if some of its parent have been unmounted, the child could actually + // still be attached to its parent, and this may be important if we + // want to remount the parent, because the vdom need to match the + // actual DOM + child.__destroy(childOwl.parent); + if (childOwl.pvnode) { + // we remove the key here to make sure that the patching algorithm + // is able to make the difference between this pvnode and an eventual + // other instance of the same component + delete childOwl.pvnode.key; + // Since the component has been unmounted, we do not want to actually + // call a remove hook. This is pretty important, since the t-component + // directive actually disabled it, so the vdom algorithm will just + // not remove the child elm if we don't remove the hook. + delete childOwl.pvnode.data!.hook!.remove; + } } } if (!vnode) { diff --git a/src/qweb/base_directives.ts b/src/qweb/base_directives.ts index feff7b2c..95c9a3cd 100644 --- a/src/qweb/base_directives.ts +++ b/src/qweb/base_directives.ts @@ -275,16 +275,12 @@ QWeb.addDirective({ const parentNode = ctx.parentNode ? `c${ctx.parentNode}` : "result"; const extra = `Object.assign({}, extra, {parentNode: ${parentNode}, parent: ${parentComponent}, key: ${key}})`; if (ctx.parentNode) { - ctx.addLine( - `this.constructor.subTemplates['${subId}'].call(this, scope, ${extra});` - ); + ctx.addLine(`this.constructor.subTemplates['${subId}'].call(this, scope, ${extra});`); } else { // this is a t-call with no parentnode, we need to extract the result ctx.rootContext.shouldDefineResult = true; ctx.addLine(`result = []`); - ctx.addLine( - `this.constructor.subTemplates['${subId}'].call(this, scope, ${extra});` - ); + ctx.addLine(`this.constructor.subTemplates['${subId}'].call(this, scope, ${extra});`); ctx.addLine(`result = result[0]`); } diff --git a/src/qweb/qweb.ts b/src/qweb/qweb.ts index 618807ec..d9e6b067 100644 --- a/src/qweb/qweb.ts +++ b/src/qweb/qweb.ts @@ -216,8 +216,8 @@ export class QWeb extends EventBus { // id, and a (global) mapping from an id to the compiled function. This is // necessary to ensure that global templates can be called with more than one // QWeb instance. - subTemplates: {[key: string]: number} = {}; - static subTemplates: {[id: number]: Function} = {}; + subTemplates: { [key: string]: number } = {}; + static subTemplates: { [id: number]: Function } = {}; isUpdating: boolean = false; translateFn?: QWebConfig["translateFn"]; diff --git a/tests/component/component.test.ts b/tests/component/component.test.ts index 3500c2b7..e731c1c3 100644 --- a/tests/component/component.test.ts +++ b/tests/component/component.test.ts @@ -3021,7 +3021,7 @@ describe("random stuff/miscellaneous", () => { static template = xml`
__ -
` + `; } class Child extends Component { static components = { Custom }; @@ -3029,7 +3029,7 @@ describe("random stuff/miscellaneous", () => { ` + subKey="props.subKey"/>`; } class Parent extends Component { @@ -3045,24 +3045,24 @@ describe("random stuff/miscellaneous", () => { } const parent = new Parent(null); await parent.mount(fixture); - expect(fixture.textContent!.trim()).toBe('1__1'); + expect(fixture.textContent!.trim()).toBe("1__1"); // First step: change the Custom's instance Object.assign(parent.childProps, { - subKey: 2, + subKey: 2, }); parent.render(); await nextTick(); - expect(fixture.textContent!.trim()).toBe('1__2'); + expect(fixture.textContent!.trim()).toBe("1__2"); // Second step, change both Child's and Custom's instance Object.assign(parent.childProps, { - key: 2, - subKey: 3, + key: 2, + subKey: 3, }); parent.render(); await nextTick(); - expect(fixture.textContent!.trim()).toBe('2__3'); + expect(fixture.textContent!.trim()).toBe("2__3"); }); }); diff --git a/tests/component/un_mounting.test.ts b/tests/component/un_mounting.test.ts index 3c6730c8..10411a1e 100644 --- a/tests/component/un_mounting.test.ts +++ b/tests/component/un_mounting.test.ts @@ -509,4 +509,103 @@ describe("unmounting and remounting", () => { expect(error).toBeDefined(); expect(error.message).toBe("Cannot mount a destroyed component"); }); + + test("destroying a sub-component cleans itself from parent's vnode", async () => { + class C1 extends Component { + static template = xml`
`; + } + class P extends Component { + static components = { C1 }; + static template = xml`
`; + state = { + a: "first", + }; + } + const parent = new P(); + await parent.mount(fixture); + expect(fixture.textContent).toBe("first"); + parent.unmount(); + parent.state.a = ""; + parent.mount(fixture); + parent.state.a = "fixed"; + await parent.render(); + expect(fixture.textContent).toBe("fixed"); + }); + + test("destroying a sub-component cleans itself from parent's vnode, part 2", async () => { + class C1 extends Component { + static template = xml`
`; + } + class P extends Component { + static components = { C1 }; + static template = xml`
some text
`; + state = { + a: "first", + }; + } + const parent = new P(); + await parent.mount(fixture); + expect(fixture.textContent).toBe("firstsome text"); + parent.unmount(); + parent.state.a = ""; + parent.mount(fixture); + parent.state.a = "fixed"; + await parent.render(); + expect(fixture.textContent).toBe("fixedsome text"); + }); + + test("destroying a sub-component cleans itself from parent's vnode, part 3", async () => { + class C1 extends Component { + static template = xml`
`; + } + + class C2 extends Component { + static template = xml``; + static components = { C1 }; + } + + class P extends Component { + static components = { C2 }; + static template = xml`
`; + state = { + a: "first", + }; + } + const parent = new P(); + await parent.mount(fixture); + expect(fixture.textContent).toBe("first"); + parent.unmount(); + parent.state.a = ""; + parent.mount(fixture); + parent.state.a = "fixed"; + await parent.render(); + expect(fixture.textContent).toBe("fixed"); + }); + + test("destroying a sub-component cleans itself from parent's vnode, part 4", async () => { + class C1 extends Component { + static template = xml`
`; + } + + class C2 extends Component { + static template = xml``; + static components = { C1 }; + } + class P extends Component { + static components = { C2 }; + static template = xml`
some text
`; + state = { + a: "first", + }; + } + const parent = new P(); + await parent.mount(fixture); + expect(fixture.textContent).toBe("firstsome text"); + parent.unmount(); + parent.state.a = ""; + parent.mount(fixture); + parent.state.a = "fixed"; + await parent.render(); + expect(fixture.textContent).toBe("fixedsome text"); + }); }); diff --git a/tests/vdom.test.ts b/tests/vdom.test.ts index e1de7dff..d4a12517 100644 --- a/tests/vdom.test.ts +++ b/tests/vdom.test.ts @@ -1107,9 +1107,9 @@ describe("html to vdom", function () { }); test("svg", function () { - const nodeList = htmlToVDOM(``); - expect(nodeList).toHaveLength(1); - elm = patch(vnode0, nodeList[0]).elm; - expect(elm).toBeInstanceOf(SVGSVGElement); + const nodeList = htmlToVDOM(``); + expect(nodeList).toHaveLength(1); + elm = patch(vnode0, nodeList[0]).elm; + expect(elm).toBeInstanceOf(SVGSVGElement); }); });