From 1a20cc57dec807eadb8f11361a822ea07dff22cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Wed, 13 Jan 2021 10:40:02 +0100 Subject: [PATCH] [FIX] component can be updated while detached from the main DOM This commit tries to improve the interactions involving unmounted components, or components mounted in an htmelement which is detached from the main DOM, and rendering actions. The main example is mounting a component in detached div, to prepare all children. If we just mount the component, it will work as expected: the full component tree is rendered in memory, and ready to be really mounted at the desired target. However, if before doing that, we update the component and call render on it (for example, with a change in an observed state), then this rendering will be ignored, and therefore, the full subcomponent tree is not uptodate. This commit will also solve another issue in the compatibility layer in odoo: in the form renderer, we mount components with the adapter in a div, which is not yet attached to the DOM. We then manually call the mounted hook when on_attach_callback is called. This means that before this commit, any change to the components between the initial rendering and the call to mounted will be ignored. As a bonus, this commit has the effect of bringing closer the semantics of render and mount operations, which is certainly good. closes #823 --- src/component/component.ts | 17 ++++---- src/component/fiber.ts | 39 +++++++++-------- tests/component/component.test.ts | 63 ++++++++++++++++++++++++++++ tests/component/un_mounting.test.ts | 32 ++++++++++++++ tests/tooling/debug_script_3.test.ts | 8 +++- tools/debug.js | 2 +- 6 files changed, 132 insertions(+), 29 deletions(-) diff --git a/src/component/component.ts b/src/component/component.ts index 35da0c16..338b75d3 100644 --- a/src/component/component.ts +++ b/src/component/component.ts @@ -322,7 +322,14 @@ export class Component { } if (__owl__.currentFiber) { const currentFiber = __owl__.currentFiber; - if (currentFiber.target === target && currentFiber.position === position) { + if (!currentFiber.target && !currentFiber.position) { + // this means we have a pending rendering, but it was a render operation, + // not a mount operation. We can simply update the fiber with the target + // and the position + currentFiber.target = target; + currentFiber.position = position; + return scheduler.addFiber(currentFiber); + } else if (currentFiber.target === target && currentFiber.position === position) { return scheduler.addFiber(currentFiber); } else { scheduler.rejectFiber(currentFiber, "Mounting operation cancelled"); @@ -366,12 +373,6 @@ export class Component { async render(force: boolean = false): Promise { const __owl__ = this.__owl__; const currentFiber = __owl__.currentFiber; - if (!__owl__.isMounted && !currentFiber) { - // if we get here, this means that the component was either never mounted, - // or was unmounted and some state change triggered a render. Either way, - // we do not want to actually render anything in this case. - return; - } if (currentFiber && !currentFiber.isRendered && !currentFiber.isCompleted) { return scheduler.addFiber(currentFiber.root); } @@ -385,8 +386,6 @@ export class Component { if (fiber.isCompleted) { return; } - // we are mounted (__owl__.isMounted), or if we are currently being - // mounted (!isMounted), so we call __render this.__render(fiber); } else { // we were mounted when render was called, but we aren't anymore, so we diff --git a/src/component/fiber.ts b/src/component/fiber.ts index b19d10eb..9433f95f 100644 --- a/src/component/fiber.ts +++ b/src/component/fiber.ts @@ -188,7 +188,8 @@ export class Fiber { complete() { let component = this.component; this.isCompleted = true; - if (!this.target && !component.__owl__.isMounted) { + const { isMounted, isDestroyed } = component.__owl__; + if (isDestroyed) { return; } @@ -202,14 +203,16 @@ export class Fiber { const patchLen = patchQueue.length; // call willPatch hook on each fiber of patchQueue - for (let i = 0; i < patchLen; i++) { - const fiber = patchQueue[i]; - if (fiber.shouldPatch) { - component = fiber.component; - if (component.__owl__.willPatchCB) { - component.__owl__.willPatchCB(); + if (isMounted) { + for (let i = 0; i < patchLen; i++) { + const fiber = patchQueue[i]; + if (fiber.shouldPatch) { + component = fiber.component; + if (component.__owl__.willPatchCB) { + component.__owl__.willPatchCB(); + } + component.willPatch(); } - component.willPatch(); } } @@ -271,16 +274,18 @@ export class Fiber { } // call patched/mounted hook on each fiber of (reversed) patchQueue - for (let i = patchLen - 1; i >= 0; i--) { - const fiber = patchQueue[i]; - component = fiber.component; - if (fiber.shouldPatch && !this.target) { - component.patched(); - if (component.__owl__.patchedCB) { - component.__owl__.patchedCB(); + if (isMounted || inDOM) { + for (let i = patchLen - 1; i >= 0; i--) { + const fiber = patchQueue[i]; + component = fiber.component; + if (fiber.shouldPatch && !this.target) { + component.patched(); + if (component.__owl__.patchedCB) { + component.__owl__.patchedCB(); + } + } else { + component.__callMounted(); } - } else if (this.target ? inDOM : true) { - component.__callMounted(); } } } diff --git a/tests/component/component.test.ts b/tests/component/component.test.ts index 6d17f08c..f7cdf8af 100644 --- a/tests/component/component.test.ts +++ b/tests/component/component.test.ts @@ -962,6 +962,69 @@ describe("lifecycle hooks", () => { "parent:patched", ]); }); + + test("willPatch/patched hook is not called if not mounted in DOM", async () => { + const steps: string[] = []; + + class ChildWidget extends Component { + static template = xml`
`; + constructor(parent, props) { + super(parent, props); + steps.push("child:constructor"); + } + mounted() { + steps.push("child:mounted"); + } + willPatch() { + steps.push("child:willPatch"); + } + patched() { + steps.push("child:patched"); + } + } + class ParentWidget extends Component { + static template = xml` +
+ +
+ `; + static components = { child: ChildWidget }; + state = useState({ n: 1 }); + constructor() { + super(); + steps.push("parent:constructor"); + } + mounted() { + steps.push("parent:mounted"); + } + willPatch() { + steps.push("parent:willPatch"); + } + patched() { + steps.push("parent:patched"); + } + } + + const div = document.createElement("div"); + const widget = new ParentWidget(); + await widget.mount(div); + expect(steps).toEqual(["parent:constructor", "child:constructor"]); + + widget.state.n = 2; + await nextTick(); + + expect(steps).toEqual(["parent:constructor", "child:constructor"]); + + // then we remount the component in the dom + await widget.mount(fixture); + + expect(steps).toEqual([ + "parent:constructor", + "child:constructor", + "child:mounted", + "parent:mounted", + ]); + }); }); describe("destroy method", () => { diff --git a/tests/component/un_mounting.test.ts b/tests/component/un_mounting.test.ts index b964721b..ed6b7481 100644 --- a/tests/component/un_mounting.test.ts +++ b/tests/component/un_mounting.test.ts @@ -323,6 +323,38 @@ describe("unmounting and remounting", () => { expect(steps).toEqual([2, 2, 3]); }); + test("change state and render while mounted in detached dom", async () => { + class App extends Component { + static template = xml`
`; + state = useState({ val: 1 }); + } + + const detachedDiv = document.createElement("div"); + const app = await mount(App, { target: detachedDiv }); + + expect(detachedDiv.innerHTML).toBe("
1
"); + app.state.val = 2; + await nextTick(); + expect(detachedDiv.innerHTML).toBe("
2
"); + }); + + test("destroy and change state after mounted in detached dom", async () => { + class App extends Component { + static template = xml`
`; + state = useState({ val: 1 }); + } + + const detachedDiv = document.createElement("div"); + const app = await mount(App, { target: detachedDiv }); + + expect(detachedDiv.innerHTML).toBe("
1
"); + + app.destroy(); + app.state.val = 2; + await nextTick(); + expect(detachedDiv.innerHTML).toBe(""); + }); + test("change state while component is unmounted", async () => { let child; class Child extends Component { diff --git a/tests/tooling/debug_script_3.test.ts b/tests/tooling/debug_script_3.test.ts index a978fdd5..bda9c655 100644 --- a/tests/tooling/debug_script_3.test.ts +++ b/tests/tooling/debug_script_3.test.ts @@ -8,7 +8,7 @@ import * as owl from "../../src/index"; import { Component, Env } from "../../src/component/component"; import { xml } from "../../src/tags"; -import { makeTestFixture, makeTestEnv } from "../helpers"; +import { makeTestFixture, makeTestEnv, nextTick } from "../helpers"; let fixture: HTMLElement = makeTestFixture(); let env: Env = makeTestEnv(); @@ -31,6 +31,7 @@ test("log a specific message for render method calls if component is not mounted parent.unmount(); parent.state.value = 2; + await nextTick(); expect(steps).toEqual([ "[OWL_DEBUG] Parent constructor, props={}", "[OWL_DEBUG] Parent mount", @@ -40,7 +41,10 @@ test("log a specific message for render method calls if component is not mounted "[OWL_DEBUG] Parent mounted", "[OWL_DEBUG] scheduler: stop running tasks queue", "[OWL_DEBUG] Parent willUnmount", - "[OWL_DEBUG] Parent render (warning: component is not mounted, this render has no effect)", + "[OWL_DEBUG] Parent render (warning: component is not mounted)", + "[OWL_DEBUG] scheduler: start running tasks queue", + "[OWL_DEBUG] Parent rendering template", + "[OWL_DEBUG] scheduler: stop running tasks queue", ]); console.log = log; }); diff --git a/tools/debug.js b/tools/debug.js index d1df8d07..3d38902c 100644 --- a/tools/debug.js +++ b/tools/debug.js @@ -102,7 +102,7 @@ const __owl__ = component.__owl__; let msg = `render`; if (!__owl__.isMounted && !__owl__.currentFiber) { - msg += ` (warning: component is not mounted, this render has no effect)`; + msg += ` (warning: component is not mounted)`; } log(msg); return render(...args);