From e56580efb23f039c9232d2b06f85fb7e0fdbc55d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Sat, 27 Apr 2019 19:20:29 +0200 Subject: [PATCH] [FIX] component: call willPatch/patched hooks properly Each component called its hooks properly individually, however the issue was that it was done component by component. However, in practice, we want the willPatch hooks to be done before ANY patch is actually performed, and the patched should be called AFTER every patch is applied. The trick is to propagate a list of patched components across various function calls. closes #64 --- src/component.ts | 42 ++++++--- src/qweb.ts | 4 +- tests/__snapshots__/component.test.ts.snap | 12 +-- tests/component.test.ts | 101 +++++++++++++++++---- tests/qweb.test.ts | 5 + 5 files changed, 127 insertions(+), 37 deletions(-) diff --git a/src/component.ts b/src/component.ts index 8e6000ff..981d269d 100644 --- a/src/component.ts +++ b/src/component.ts @@ -236,17 +236,33 @@ export class Component< } } - async render(force: boolean = false): Promise { + async render(force: boolean = false, patchQueue?: any[]): Promise { if (this.__owl__.isDestroyed) { return; } - const renderVDom = this._render(force); + const shouldCallPatchHooks: boolean = !patchQueue; + if (shouldCallPatchHooks) { + patchQueue = []; + } + const renderVDom = this._render(force, patchQueue); const renderId = this.__owl__.renderId; const vnode = await renderVDom; if (renderId === this.__owl__.renderId) { // we only update the vnode and the actual DOM if no other rendering // occurred between now and when the render method was initially called. + if (shouldCallPatchHooks) { + for (let i = 0; i < patchQueue!.length; i++) { + const c = patchQueue![i]; + c.__owl__.willPatchVal = c.willPatch(); + } + } this._patch(vnode); + if (shouldCallPatchHooks) { + for (let i = patchQueue!.length - 1; i >= 0; i--) { + const c = patchQueue![i]; + c.patched(c.__owl__.willPatchVal); + } + } } } @@ -305,10 +321,11 @@ export class Component< async updateProps( nextProps: Props, - forceUpdate: boolean = false + forceUpdate: boolean = false, + patchQueue: any[], ): Promise { const shouldUpdate = forceUpdate || this.shouldUpdate(nextProps); - return shouldUpdate ? this._updateProps(nextProps) : Promise.resolve(); + return shouldUpdate ? this._updateProps(nextProps, patchQueue) : Promise.resolve(); } set(target: any, key: string | number, value: any) { @@ -319,21 +336,16 @@ export class Component< // Private //-------------------------------------------------------------------------- - async _updateProps(nextProps: Props): Promise { + async _updateProps(nextProps: Props, patchQueue: any[]): Promise { await this.willUpdateProps(nextProps); this.props = nextProps; - await this.render(); + await this.render(false, patchQueue); } _patch(vnode) { this.__owl__.renderPromise = null; if (this.__owl__.vnode) { - const isMounted = this.__owl__.isMounted; - const snapshot = isMounted && this.willPatch(); this.__owl__.vnode = patch(this.__owl__.vnode, vnode); - if (isMounted) { - this.patched(snapshot); - } } else { this.__owl__.vnode = patch(document.createElement(vnode.sel!), vnode); } @@ -358,7 +370,10 @@ export class Component< return this.__owl__.renderPromise; } - async _render(force: boolean = false): Promise { + async _render(force: boolean = false, patchQueue: any[] = []): Promise { + if (this.__owl__.isMounted) { + patchQueue.push(this) + } this.__owl__.renderId++; const promises: Promise[] = []; const template = this.inlineTemplate || this.template; @@ -368,7 +383,8 @@ export class Component< let vnode = this.env.qweb.render(template, this, { promises, handlers: this.__owl__.boundHandlers, - forceUpdate: force + forceUpdate: force, + patchQueue, }); if (this.__owl__.observer) { this.__owl__.observer.allowMutations = true; diff --git a/src/qweb.ts b/src/qweb.ts index 49d8fbc7..4cc34316 100644 --- a/src/qweb.ts +++ b/src/qweb.ts @@ -1060,7 +1060,7 @@ const widgetDirective: Directive = { ctx.addIf(`w${widgetID} && w${widgetID}.__owl__.renderPromise`); ctx.addIf(`w${widgetID}.__owl__.isStarted`); ctx.addLine( - `def${defID} = w${widgetID}.updateProps(props${widgetID}, extra.forceUpdate);` + `def${defID} = w${widgetID}.updateProps(props${widgetID}, extra.forceUpdate, extra.patchQueue);` ); ctx.addElse(); ctx.addLine(`isNew${widgetID} = true`); @@ -1076,7 +1076,7 @@ const widgetDirective: Directive = { ctx.addIf(`!def${defID}`); ctx.addIf(`w${widgetID}`); ctx.addLine( - `def${defID} = w${widgetID}.updateProps(props${widgetID}, extra.forceUpdate);` + `def${defID} = w${widgetID}.updateProps(props${widgetID}, extra.forceUpdate, extra.patchQueue);` ); ctx.addElse(); ctx.addLine( diff --git a/tests/__snapshots__/component.test.ts.snap b/tests/__snapshots__/component.test.ts.snap index b87f8a3e..3c092a63 100644 --- a/tests/__snapshots__/component.test.ts.snap +++ b/tests/__snapshots__/component.test.ts.snap @@ -30,7 +30,7 @@ exports[`composition sub widgets with some state rendered in a loop 1`] = ` let isNew7 = !w7; if (w7 && w7.__owl__.renderPromise) { if (w7.__owl__.isStarted) { - def6 = w7.updateProps(props7, extra.forceUpdate); + def6 = w7.updateProps(props7, extra.forceUpdate, extra.patchQueue); } else { isNew7 = true if (props7 === w7.__owl__.renderProps) { @@ -43,7 +43,7 @@ exports[`composition sub widgets with some state rendered in a loop 1`] = ` } if (!def6) { if (w7) { - def6 = w7.updateProps(props7, extra.forceUpdate); + def6 = w7.updateProps(props7, extra.forceUpdate, extra.patchQueue); } else { w7 = new context.widgets['ChildWidget'](owner, props7); context.__owl__.cmap[key8] = w7.__owl__.id; @@ -78,7 +78,7 @@ exports[`random stuff/miscellaneous snapshotting compiled code 1`] = ` let isNew4 = !w4; if (w4 && w4.__owl__.renderPromise) { if (w4.__owl__.isStarted) { - def3 = w4.updateProps(props4, extra.forceUpdate); + def3 = w4.updateProps(props4, extra.forceUpdate, extra.patchQueue); } else { isNew4 = true if (props4 === w4.__owl__.renderProps) { @@ -91,7 +91,7 @@ exports[`random stuff/miscellaneous snapshotting compiled code 1`] = ` } if (!def3) { if (w4) { - def3 = w4.updateProps(props4, extra.forceUpdate); + def3 = w4.updateProps(props4, extra.forceUpdate, extra.patchQueue); } else { w4 = new context.widgets['child'](owner, props4); context.__owl__.cmap[key5] = w4.__owl__.id; @@ -124,7 +124,7 @@ exports[`random stuff/miscellaneous t-props should not be undefined (snapshottin let isNew4 = !w4; if (w4 && w4.__owl__.renderPromise) { if (w4.__owl__.isStarted) { - def3 = w4.updateProps(props4, extra.forceUpdate); + def3 = w4.updateProps(props4, extra.forceUpdate, extra.patchQueue); } else { isNew4 = true if (props4 === w4.__owl__.renderProps) { @@ -137,7 +137,7 @@ exports[`random stuff/miscellaneous t-props should not be undefined (snapshottin } if (!def3) { if (w4) { - def3 = w4.updateProps(props4, extra.forceUpdate); + def3 = w4.updateProps(props4, extra.forceUpdate, extra.patchQueue); } else { w4 = new context.widgets['child'](owner, props4); context.__owl__.cmap[4] = w4.__owl__.id; diff --git a/tests/component.test.ts b/tests/component.test.ts index 6b8692dd..e8e32c1f 100644 --- a/tests/component.test.ts +++ b/tests/component.test.ts @@ -264,6 +264,55 @@ describe("lifecycle hooks", () => { ]); }); + test("willPatch, patched hook are called on subsubwidgets, in proper order", async () => { + const steps: any[] = []; + + class ParentWidget extends Widget { + inlineTemplate = `
>
`; + widgets = { child: ChildWidget }; + state = { n: 1 }; + willPatch() { + steps.push("parent:willPatch"); + } + patched() { + steps.push("parent:patched"); + } + } + class ChildWidget extends Widget { + inlineTemplate = `
`; + widgets = { childchild: ChildChildWidget }; + willPatch() { + steps.push("child:willPatch"); + } + patched() { + steps.push("child:patched"); + } + } + class ChildChildWidget extends Widget { + inlineTemplate = `
`; + willPatch() { + steps.push("childchild:willPatch"); + } + patched() { + steps.push("childchild:patched"); + } + } + const widget = new ParentWidget(env); + await widget.mount(fixture); + expect(steps).toEqual([]); + widget.state.n = 2; + await nextTick(); + widget.destroy(); + expect(steps).toEqual([ + "parent:willPatch", + "child:willPatch", + "childchild:willPatch", + "childchild:patched", + "child:patched", + "parent:patched" + ]); + }); + test("willStart, mounted on subwidget rendered after main is mounted in some other position", async () => { const steps: string[] = []; @@ -450,6 +499,12 @@ describe("lifecycle hooks", () => { test("willUpdateProps hook is called", async () => { let def = makeDeferred(); + class Parent extends Widget { + inlineTemplate = + ''; + state = { n: 1 }; + widgets = { Child: HookWidget }; + } class HookWidget extends Widget { inlineTemplate = ''; @@ -458,15 +513,15 @@ describe("lifecycle hooks", () => { return def; } } - const widget = new HookWidget(env, { n: 1 }); + const widget = new Parent(env); await widget.mount(fixture); - expect(fixture.innerHTML).toBe("1"); - widget.updateProps({ n: 2 }); + expect(fixture.innerHTML).toBe("1"); + widget.state.n = 2; await nextTick(); - expect(fixture.innerHTML).toBe("1"); + expect(fixture.innerHTML).toBe("1"); def.resolve(); await nextTick(); - expect(fixture.innerHTML).toBe("2"); + expect(fixture.innerHTML).toBe("2"); }); test("patched hook is called after updating State", async () => { @@ -495,16 +550,23 @@ describe("lifecycle hooks", () => { test("patched hook is called after updateProps", async () => { let n = 0; + class Parent extends Widget { + inlineTemplate = '
'; + state = { a: 1 }; + widgets = { Child: TestWidget }; + } + class TestWidget extends Widget { patched() { n++; } } - const widget = new TestWidget(env, { a: 1 }); + const widget = new Parent(env); await widget.mount(fixture); expect(n).toBe(0); - await widget.updateProps({ a: 2 }); + widget.state.a = 2; + await nextTick(); expect(n).toBe(1); }); @@ -528,20 +590,27 @@ describe("lifecycle hooks", () => { test("shouldUpdate hook prevent rerendering", async () => { let shouldUpdate = false; + class Parent extends Widget { + inlineTemplate = `
`; + state = { val: 42 }; + widgets = { Child: TestWidget }; + } class TestWidget extends Widget { inlineTemplate = `
`; shouldUpdate() { return shouldUpdate; } } - const widget = new TestWidget(env, { val: 42 }); + const widget = new Parent(env); await widget.mount(fixture); - expect(fixture.innerHTML).toBe("
42
"); - await widget.updateProps({ val: 123 }); - expect(fixture.innerHTML).toBe("
42
"); + expect(fixture.innerHTML).toBe("
42
"); + widget.state.val = 123; + await nextTick(); + expect(fixture.innerHTML).toBe("
42
"); shouldUpdate = true; - await widget.updateProps({ val: 666 }); - expect(fixture.innerHTML).toBe("
666
"); + widget.state.val = 666; + await nextTick(); + expect(fixture.innerHTML).toBe("
666
"); }); test("sub widget (inside sub node): hooks are correctly called", async () => { @@ -613,9 +682,9 @@ describe("lifecycle hooks", () => { // Not sure about this order. If you disagree, feel free to open an issue... expect(steps).toEqual([ + "parent:willPatch", "child:willPatch", "child:patched", - "parent:willPatch", "parent:patched" ]); }); @@ -1416,8 +1485,8 @@ describe("async rendering", () => { class ChildA extends Widget { inlineTemplate = `a`; - updateProps(props): Promise { - return defA.then(() => super.updateProps(props)); + updateProps(props, forceUpdate, fiber): Promise { + return defA.then(() => super.updateProps(props, forceUpdate, fiber)); } } class ChildB extends Widget { diff --git a/tests/qweb.test.ts b/tests/qweb.test.ts index d89b08ba..4670ee46 100644 --- a/tests/qweb.test.ts +++ b/tests/qweb.test.ts @@ -1097,12 +1097,17 @@ describe("t-key", () => { describe("debugging", () => { test("t-debug", () => { + const consoleLog = console.log; + console.log = jest.fn() qweb.addTemplate( "test", `
hey
` ); qweb.render('test'); expect(qweb.templates.test.toString()).toMatchSnapshot(); + + expect(console.log).toHaveBeenCalledTimes(1); + console.log = consoleLog; }); test("t-log", () => {