[FIX] component: cancel previous mounting operations if necessary

closes #626

src/component/component.ts

@@ -37,8 +37,10 @@ export interface Env {
   [key: string]: any;
 }
 
+export type MountPosition = "first-child" | "last-child" | "self";
+
 interface MountOptions {
-  position?: "first-child" | "last-child" | "self";
+  position?: MountPosition;
 }
 
 /**
@@ -106,7 +108,6 @@ export class Component<Props extends {} = any, T extends Env = Env> {
   static env: any = {};
   // expose scheduler s.t. it can be mocked for testing purposes
   static scheduler: Scheduler = scheduler;
-  __target: HTMLElement | undefined;
 
   /**
    * The `el` is the root element of the component.  Note that it could be null:
@@ -311,21 +312,20 @@ export class Component<Props extends {} = any, T extends Env = Env> {
         return Promise.resolve();
       }
     }
+    if (__owl__.currentFiber) {
+      const currentFiber = __owl__.currentFiber;
+      if (currentFiber.target === target && currentFiber.position === position) {
+        return scheduler.addFiber(currentFiber);
+      } else {
+        scheduler.rejectFiber(currentFiber, "Mounting operation cancelled");
+      }
+    }
     if (!(target instanceof HTMLElement || target instanceof DocumentFragment)) {
       let message = `Component '${this.constructor.name}' cannot be mounted: the target is not a valid DOM node.`;
       message += `\nMaybe the DOM is not ready yet? (in that case, you can use owl.utils.whenReady)`;
       throw new Error(message);
     }
-    let inserter =
+    const fiber = new Fiber(null, this, false, target, position);
-      position === "last-child"
-        ? el => target.appendChild(el)
-        : position === "first-child"
-        ? el => target.prepend(el)
-        : el => {};
-    if (position === "self") {
-      this.__target = target as HTMLElement;
-    }
-    const fiber = new Fiber(null, this, false, inserter);
     fiber.shouldPatch = false;
     if (!__owl__.vnode) {
       this.__prepareAndRender(fiber, () => {});
@@ -370,7 +370,7 @@ export class Component<Props extends {} = any, T extends Env = Env> {
     // currentFiber that is already rendered (isRendered is true), so we are
     // about to be mounted
     const isMounted = __owl__.isMounted;
-    const fiber = new Fiber(null, this, force, null);
+    const fiber = new Fiber(null, this, force, null, null);
     Promise.resolve().then(() => {
       if (__owl__.isMounted || !isMounted) {
         if (fiber.isCompleted) {
@@ -527,7 +527,7 @@ export class Component<Props extends {} = any, T extends Env = Env> {
     const shouldUpdate = parentFiber.force || this.shouldUpdate(nextProps);
     if (shouldUpdate) {
       const __owl__ = this.__owl__;
-      const fiber = new Fiber(parentFiber, this, parentFiber.force, null);
+      const fiber = new Fiber(parentFiber, this, parentFiber.force, null, null);
       if (!parentFiber.child) {
         parentFiber.child = fiber;
       } else {
@@ -559,20 +559,8 @@ export class Component<Props extends {} = any, T extends Env = Env> {
    * Main patching method. We call the virtual dom patch method here to convert
    * a virtual dom vnode into some actual dom.
    */
-  __patch(vnode: VNode) {
+  __patch(target: HTMLElement | VNode | DocumentFragment, vnode: VNode) {
-    const __owl__ = this.__owl__;
+    this.__owl__.vnode = patch(target as any, vnode);
-    if (this.__target) {
-      if (this.__target.tagName.toLowerCase() !== vnode.sel) {
-        throw new Error(
-          `Cannot attach '${this.constructor.name}' to target node (not same tag name)`
-        );
-      }
-      __owl__.vnode = patch(this.__target, vnode);
-      delete this.__target;
-    } else {
-      const target = __owl__.vnode || document.createElement(vnode.sel!);
-      __owl__.vnode = patch(target, vnode);
-    }
   }
 
   /**
@@ -582,7 +570,7 @@ export class Component<Props extends {} = any, T extends Env = Env> {
    */
   __prepare(parentFiber: Fiber, scope: any, cb: CallableFunction): Fiber {
     this.__owl__.scope = scope;
-    const fiber = new Fiber(parentFiber, this, parentFiber.force, null);
+    const fiber = new Fiber(parentFiber, this, parentFiber.force, null, null);
     fiber.shouldPatch = false;
     if (!parentFiber.child) {
       parentFiber.child = fiber;

src/component/directive.ts

@@ -368,7 +368,7 @@ QWeb.addDirective({
     if (transition) {
       ctx.addLine(`const __patch${componentID} = w${componentID}.__patch;`);
       ctx.addLine(
-        `w${componentID}.__patch = fiber => {__patch${componentID}.call(w${componentID}, fiber); if(!w${componentID}.__owl__.transitionInserted){w${componentID}.__owl__.transitionInserted = true;utils.transitionInsert(w${componentID}.__owl__.vnode, '${transition}');}};`
+        `w${componentID}.__patch = (t, vn) => {__patch${componentID}.call(w${componentID}, t, vn); if(!w${componentID}.__owl__.transitionInserted){w${componentID}.__owl__.transitionInserted = true;utils.transitionInsert(w${componentID}.__owl__.vnode, '${transition}');}};`
       );
     }
     ctx.addLine(`parent.__owl__.cmap[${templateKey}] = w${componentID}.__owl__.id;`);

src/component/fiber.ts

@@ -1,5 +1,5 @@
 import { h, VNode } from "../vdom/index";
-import { Component } from "./component";
+import { Component, MountPosition } from "./component";
 import { scheduler } from "./scheduler";
 
 /**
@@ -46,7 +46,8 @@ export class Fiber {
   // scheduler.
   counter: number = 0;
 
-  inserter: (el: HTMLElement) => void | null;
+  target: HTMLElement | DocumentFragment | null;
+  position: MountPosition | null;
 
   scope: any;
 
@@ -61,10 +62,17 @@ export class Fiber {
 
   error?: Error;
 
-  constructor(parent: Fiber | null, component: Component, force: boolean, inserter) {
+  constructor(
+    parent: Fiber | null,
+    component: Component,
+    force: boolean,
+    target: HTMLElement | DocumentFragment | null,
+    position: MountPosition | null
+  ) {
     this.component = component;
     this.force = force;
-    this.inserter = inserter;
+    this.target = target;
+    this.position = position;
 
     const __owl__ = component.__owl__;
     this.scope = __owl__.scope;
@@ -179,7 +187,7 @@ export class Fiber {
   complete() {
     let component = this.component;
     this.isCompleted = true;
-    if (!this.inserter && !component.__owl__.isMounted) {
+    if (!this.target && !component.__owl__.isMounted) {
       return;
     }
 
@@ -208,17 +216,41 @@ export class Fiber {
     for (let i = patchLen - 1; i >= 0; i--) {
       const fiber = patchQueue[i];
       component = fiber.component;
-      component.__patch(fiber.vnode!);
+      if (fiber.target && i === 0) {
-      if (!fiber.shouldPatch && (!fiber.inserter || i !== 0)) {
+        let target;
-        component.__owl__.pvnode!.elm = component.__owl__.vnode!.elm;
+        if (fiber.position === "self") {
+          target = fiber.target;
+          if ((target as HTMLElement).tagName.toLowerCase() !== fiber.vnode!.sel) {
+            throw new Error(
+              `Cannot attach '${component.constructor.name}' to target node (not same tag name)`
+            );
+          }
+        } else {
+          target = component.__owl__.vnode || document.createElement(fiber.vnode!.sel!);
+        }
+        component.__patch(target!, fiber.vnode!);
+      } else {
+        if (fiber.shouldPatch) {
+          component.__patch(component.__owl__.vnode!, fiber.vnode!);
+        } else {
+          component.__patch(document.createElement(fiber.vnode!.sel!), fiber.vnode!);
+          component.__owl__.pvnode!.elm = component.__owl__.vnode!.elm;
+        }
       }
       component.__owl__.currentFiber = null;
     }
 
     // insert into the DOM (mount case)
     let inDOM = false;
-    if (this.inserter) {
+    if (this.target) {
-      this.inserter(this.component.el!);
+      switch (this.position) {
+        case "first-child":
+          this.target.prepend(this.component.el!);
+          break;
+        case "last-child":
+          this.target.appendChild(this.component.el!);
+          break;
+      }
       inDOM = document.body.contains(this.component.el);
       this.component.env.qweb.trigger("dom-appended");
     }
@@ -227,12 +259,12 @@ export class Fiber {
     for (let i = patchLen - 1; i >= 0; i--) {
       const fiber = patchQueue[i];
       component = fiber.component;
-      if (fiber.shouldPatch && !this.inserter) {
+      if (fiber.shouldPatch && !this.target) {
         component.patched();
         if (component.__owl__.patchedCB) {
           component.__owl__.patchedCB();
         }
-      } else if (this.inserter ? inDOM : true) {
+      } else if (this.target ? inDOM : true) {
         component.__callMounted();
       }
     }

src/component/scheduler.ts

@@ -34,7 +34,7 @@ export class Scheduler {
     this.isRunning = false;
   }
 
-  addFiber(fiber): Promise<void> {
+  addFiber(fiber: Fiber): Promise<void> {
     // if the fiber was remapped into a larger rendering fiber, it may not be a
     // root fiber.  But we only want to register root fibers
     fiber = fiber.root;
@@ -57,6 +57,17 @@ export class Scheduler {
     });
   }
 
+  rejectFiber(fiber: Fiber, reason: string) {
+    fiber = fiber.root;
+    const index = this.tasks.findIndex(t => t.fiber === fiber);
+    if (index >= 0) {
+      const [task] = this.tasks.splice(index, 1);
+      fiber.cancel();
+      fiber.error = new Error(reason);
+      task.callback();
+    }
+  }
+
   /**
    * Process all current tasks. This only applies to the fibers that are ready.
    * Other tasks are left unchanged.

src/misc/portal.ts

@@ -124,7 +124,7 @@ export class Portal extends Component<Props> {
    *
    * @override
    */
-  __patch(vnode) {
+  __patch(target, vnode) {
     if (this.doTargetLookUp) {
       const target = document.querySelector(this.props.target);
       if (!target) {
@@ -153,7 +153,7 @@ export class Portal extends Component<Props> {
     this.portal = patch(portalPatch, vnode.children![0] as VNode);
     vnode.children = [];
 
-    super.__patch(vnode);
+    super.__patch(target, vnode);
 
     if (shouldDeploy) {
       this.__deployPortal();

tests/__snapshots__/animations.test.ts.snap

@@ -28,7 +28,7 @@ exports[`animations t-transition combined with component 1`] = `
         if (!W2) {throw new Error('Cannot find the definition of component \\"' + componentKey2 + '\\"')}
         w2 = new W2(parent, props2);
         const __patch2 = w2.__patch;
-        w2.__patch = fiber => {__patch2.call(w2, fiber); if(!w2.__owl__.transitionInserted){w2.__owl__.transitionInserted = true;utils.transitionInsert(w2.__owl__.vnode, 'chimay');}};
+        w2.__patch = (t, vn) => {__patch2.call(w2, t, vn); if(!w2.__owl__.transitionInserted){w2.__owl__.transitionInserted = true;utils.transitionInsert(w2.__owl__.vnode, 'chimay');}};
         parent.__owl__.cmap['__3__'] = w2.__owl__.id;
         let fiber = w2.__prepare(extra.fiber, undefined, () => { const vnode = fiber.vnode; pvnode.sel = vnode.sel; });
         let pvnode = h('dummy', {key: '__3__', hook: {remove() {},destroy(vn) {let finalize = () => {
@@ -73,7 +73,7 @@ exports[`animations t-transition combined with t-component and t-if 1`] = `
             if (!W2) {throw new Error('Cannot find the definition of component \\"' + componentKey2 + '\\"')}
             w2 = new W2(parent, props2);
             const __patch2 = w2.__patch;
-            w2.__patch = fiber => {__patch2.call(w2, fiber); if(!w2.__owl__.transitionInserted){w2.__owl__.transitionInserted = true;utils.transitionInsert(w2.__owl__.vnode, 'chimay');}};
+            w2.__patch = (t, vn) => {__patch2.call(w2, t, vn); if(!w2.__owl__.transitionInserted){w2.__owl__.transitionInserted = true;utils.transitionInsert(w2.__owl__.vnode, 'chimay');}};
             parent.__owl__.cmap['__3__'] = w2.__owl__.id;
             let fiber = w2.__prepare(extra.fiber, undefined, () => { const vnode = fiber.vnode; pvnode.sel = vnode.sel; });
             let pvnode = h('dummy', {key: '__3__', hook: {remove() {},destroy(vn) {let finalize = () => {
@@ -119,7 +119,7 @@ exports[`animations t-transition combined with t-component, remove and re-add be
             if (!W2) {throw new Error('Cannot find the definition of component \\"' + componentKey2 + '\\"')}
             w2 = new W2(parent, props2);
             const __patch2 = w2.__patch;
-            w2.__patch = fiber => {__patch2.call(w2, fiber); if(!w2.__owl__.transitionInserted){w2.__owl__.transitionInserted = true;utils.transitionInsert(w2.__owl__.vnode, 'chimay');}};
+            w2.__patch = (t, vn) => {__patch2.call(w2, t, vn); if(!w2.__owl__.transitionInserted){w2.__owl__.transitionInserted = true;utils.transitionInsert(w2.__owl__.vnode, 'chimay');}};
             parent.__owl__.cmap['__3__'] = w2.__owl__.id;
             let fiber = w2.__prepare(extra.fiber, undefined, () => { const vnode = fiber.vnode; pvnode.sel = vnode.sel; });
             let pvnode = h('dummy', {key: '__3__', hook: {remove() {},destroy(vn) {let finalize = () => {

tests/component/component.test.ts

@@ -2814,9 +2814,9 @@ describe("random stuff/miscellaneous", () => {
         steps.push(`${this.name}:render`);
         return super.__render(f);
       }
-      __patch(vnode) {
+      __patch(target, vnode) {
         steps.push(`${this.name}:__patch`);
-        super.__patch(vnode);
+        super.__patch(target, vnode);
       }
       mounted() {
         steps.push(`${this.name}:mounted`);

tests/component/un_mounting.test.ts

@@ -1,7 +1,7 @@
 import { Component, Env } from "../../src/component/component";
 import { useState } from "../../src/hooks";
 import { xml } from "../../src/tags";
-import { makeDeferred, makeTestEnv, makeTestFixture, nextTick } from "../helpers";
+import { makeDeferred, makeTestEnv, makeTestFixture, nextTick, nextMicroTick } from "../helpers";
 
 //------------------------------------------------------------------------------
 // Setup and helpers
@@ -378,18 +378,8 @@ describe("unmounting and remounting", () => {
   });
 
   test("widget can be mounted on different target", async () => {
-    const steps: string[] = [];
     class MyWidget extends Component {
       static template = xml`<div>Hey</div>`;
-      async willStart() {
-        steps.push("willstart");
-      }
-      mounted() {
-        steps.push("mounted");
-      }
-      willUnmount() {
-        steps.push("willunmount");
-      }
       patched() {
         throw new Error("patched should not be called");
       }
@@ -406,4 +396,78 @@ describe("unmounting and remounting", () => {
     await w.mount(span);
     expect(fixture.innerHTML).toBe("<div></div><span><div>Hey</div></span>");
   });
+
+  test("widget can be mounted on different target, another situation", async () => {
+    const def = makeDeferred();
+    const steps: string[] = [];
+
+    class MyWidget extends Component {
+      static template = xml`<div>Hey</div>`;
+      async willStart() {
+        return def;
+      }
+      patched() {
+        throw new Error("patched should not be called");
+      }
+    }
+    const div = document.createElement("div");
+    const span = document.createElement("span");
+    fixture.appendChild(div);
+    fixture.appendChild(span);
+    const w = new MyWidget();
+
+    w.mount(div).catch(() => steps.push("1 catch"));
+
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<div></div><span></span>");
+
+    w.mount(span).then(() => steps.push("2 resolved"));
+
+    // we wait two microticks because this is the number of internal promises
+    // that need to be resolved/rejected, and because we want to prove here
+    // that the first mount operation is cancelled immediately, and not after
+    // one full tick.
+    await nextMicroTick();
+    await nextMicroTick();
+    expect(steps).toEqual(["1 catch"]);
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<div></div><span></span>");
+
+    def.resolve();
+    await nextTick();
+    expect(steps).toEqual(["1 catch", "2 resolved"]);
+    expect(fixture.innerHTML).toBe("<div></div><span><div>Hey</div></span>");
+  });
+
+  test("widget can be mounted on same target, another situation", async () => {
+    const def = makeDeferred();
+    const steps: string[] = [];
+
+    class MyWidget extends Component {
+      static template = xml`<div>Hey</div>`;
+      async willStart() {
+        return def;
+      }
+      patched() {
+        throw new Error("patched should not be called");
+      }
+    }
+    const w = new MyWidget();
+
+    w.mount(fixture).then(() => steps.push("1 resolved"));
+
+    await nextTick();
+    expect(fixture.innerHTML).toBe("");
+
+    w.mount(fixture).then(() => steps.push("2 resolved"));
+
+    await nextTick();
+    expect(steps).toEqual([]);
+    expect(fixture.innerHTML).toBe("");
+
+    def.resolve();
+    await nextTick();
+    expect(steps).toEqual(["1 resolved", "2 resolved"]);
+    expect(fixture.innerHTML).toBe("<div>Hey</div>");
+  });
 });