[FIX] component: improve unmount/remount behaviour

doc/component.md

@@ -144,12 +144,16 @@ find a template with the component name (or one of its ancestor).
 
 We explain here all the public methods of the `Component` class.
 
-- **`mount(target)`** (async): this is the main way a component's hierarchy is added to the
+- **`mount(target, renderBeforeRemount=false)`** (async): this is the main way a
-  DOM: the root component is mounted to a target HTMLElement. Obviously, this
+  component is added to the DOM: the root component is mounted to a target
-  is asynchronous, since each children need to be created as well. Most applications
+  HTMLElement. Obviously, this is asynchronous, since each children need to be
-  will need to call `mount` exactly once, on the root component.
+  created as well. Most applications will need to call `mount` exactly once, on
+  the root component.
 
-  Note that a component can be mounted and unmounted multiple times if needed.
+  The `renderBeforeRemount` argument is useful when a component is unmounted and remounted.
+  In that case, we may want to rerender the component _before_ it is remounted, if
+  we know that its state (or something in the environment, or ...) has changed.
+  In that case, it should simply set to `true`.
 
 - **`unmount()`**: in case a component need to be detached/removed from the DOM, this
   method can be used. Most applications should not call `unmount`, this is more
@@ -258,7 +262,7 @@ interface. Therefore, some care should be made to make this method as
 fast as possible.
 
 After the `willStart` method is completed, the state will be observed with a
-new `Observer`.  Then, the component will be rendered by `QWeb`.
+new `Observer`. Then, the component will be rendered by `QWeb`.
 
 #### `mounted()`
 

src/component/component.ts

@@ -278,19 +278,22 @@ export class Component<T extends Env, Props extends {}, State extends {}> {
    *
    * Note that a component can be mounted an unmounted several times
    */
-  async mount(target: HTMLElement): Promise<void> {
+  async mount(target: HTMLElement, renderBeforeRemount: boolean = false): Promise<void> {
-    if (this.__owl__.isMounted) {
+    const __owl__ = this.__owl__;
+    if (__owl__.isMounted) {
       return;
     }
-    if (!this.__owl__.vnode) {
+    if (!__owl__.vnode) {
-      // we use the fact that renderId === 1 as a way to determine that the
-      // component is mounted for the first time
       const vnode = await this.__prepare();
-      if (this.__owl__.isDestroyed) {
+      if (__owl__.isDestroyed) {
         // component was destroyed before we get here...
         return;
       }
       this.__patch(vnode);
+    } else if (renderBeforeRemount) {
+      const patchQueue = [];
+      await this.__render(false, patchQueue, undefined, undefined);
+      this.__applyPatchQueue(<any[]>patchQueue);
     }
     target.appendChild(this.el!);
 
@@ -561,9 +564,7 @@ export class Component<T extends Env, Props extends {}, State extends {}> {
     const __owl__ = this.__owl__;
     const promises: Promise<void>[] = [];
     const patch: any[] = [this];
-    if (__owl__.isMounted) {
+    patchQueue.push(patch);
-      patchQueue.push(patch);
-    }
     if (__owl__.observer) {
       __owl__.observer.allowMutations = false;
     }

src/component/directive.ts

@@ -454,7 +454,14 @@ QWeb.addDirective({
 
     ctx.addElse();
     // need to update component
-    const patchQueueCode = async ? `patchQueue${componentID}` : "extra.patchQueue";
+    let patchQueueCode = async ? `patchQueue${componentID}` : "extra.patchQueue";
+    if (keepAlive) {
+        // if we have t-keepalive="1", the component could be unmounted, but then
+        // we __updateProps is called.  This is ok, but we do not want to call
+        // the willPatch/patched hooks of the component in this case, so we
+        // disable the patch queue
+        patchQueueCode = `w${componentID}.__owl__.isMounted ? ${patchQueueCode} : []`;
+    }
     if (QWeb.dev) {
       ctx.addLine(`utils.validateProps(w${componentID}.constructor, props${componentID})`);
     }

tests/component/__snapshots__/component.test.ts.snap

@@ -447,6 +447,49 @@ exports[`class and style attributes with t-component t-att-class is properly add
 }"
 `;
 
+exports[`composition sub components dom state with t-keepalive is preserved 1`] = `
+"function anonymous(context,extra
+) {
+    let utils = this.constructor.utils;
+    let QWeb = this.constructor;
+    let parent = context;
+    let owner = context;
+    var h = this.h;
+    let c1 = [], p1 = {key:1};
+    var vn1 = h('div', p1, c1);
+    if (context['state'].ok) {
+        //COMPONENT
+        let def3;
+        let w4 = 4 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[4]] : false;
+        let _2_index = c1.length;
+        c1.push(null);
+        let props4 = {};
+        if (w4 && w4.__owl__.renderPromise && !w4.__owl__.vnode) {
+            if (utils.shallowEqual(props4, w4.__owl__.renderProps)) {
+                def3 = w4.__owl__.renderPromise;
+            } else {
+                w4.destroy();
+                w4 = false;
+            }
+        }
+        if (!w4) {
+            let componentKey4 = \`InputWidget\`;
+            let W4 = context.components && context.components[componentKey4] || QWeb.components[componentKey4];
+            if (!W4) {throw new Error('Cannot find the definition of component \\"' + componentKey4 + '\\"')}
+            w4 = new W4(parent, props4);
+            parent.__owl__.cmap[4] = w4.__owl__.id;
+            def3 = w4.__prepare();
+            def3 = def3.then(vnode=>{let pvnode=h(vnode.sel, {key: 4, hook: {insert(vn) {let nvn=w4.__mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w4.unmount();}}});c1[_2_index]=pvnode;w4.__owl__.pvnode = pvnode;});
+        } else {
+            def3 = def3 || w4.__updateProps(props4, extra.forceUpdate, w4.__owl__.isMounted ? extra.patchQueue : []);
+            def3 = def3.then(()=>{if (w4.__owl__.isDestroyed) {return};let pvnode=w4.__owl__.pvnode;pvnode.data.hook.insert = vn => {vn.elm.parentNode.replaceChild(w4.el,vn.elm);vn.elm=w4.el;w4.__remount();};c1[_2_index]=pvnode;});
+        }
+        extra.promises.push(def3);
+    }
+    return vn1;
+}"
+`;
+
 exports[`composition sub components with some state rendered in a loop 1`] = `
 "function anonymous(context,extra
 ) {
@@ -587,6 +630,49 @@ exports[`composition t-component with dynamic value 2 1`] = `
 }"
 `;
 
+exports[`lifecycle hooks willPatch/patched hook with t-keepalive 1`] = `
+"function anonymous(context,extra
+) {
+    let utils = this.constructor.utils;
+    let QWeb = this.constructor;
+    let parent = context;
+    let owner = context;
+    var h = this.h;
+    let c1 = [], p1 = {key:1};
+    var vn1 = h('div', p1, c1);
+    if (context['state'].flag) {
+        //COMPONENT
+        let def3;
+        let w4 = 4 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[4]] : false;
+        let _2_index = c1.length;
+        c1.push(null);
+        let props4 = {v:context['state'].n};
+        if (w4 && w4.__owl__.renderPromise && !w4.__owl__.vnode) {
+            if (utils.shallowEqual(props4, w4.__owl__.renderProps)) {
+                def3 = w4.__owl__.renderPromise;
+            } else {
+                w4.destroy();
+                w4 = false;
+            }
+        }
+        if (!w4) {
+            let componentKey4 = \`ChildWidget\`;
+            let W4 = context.components && context.components[componentKey4] || QWeb.components[componentKey4];
+            if (!W4) {throw new Error('Cannot find the definition of component \\"' + componentKey4 + '\\"')}
+            w4 = new W4(parent, props4);
+            parent.__owl__.cmap[4] = w4.__owl__.id;
+            def3 = w4.__prepare();
+            def3 = def3.then(vnode=>{let pvnode=h(vnode.sel, {key: 4, hook: {insert(vn) {let nvn=w4.__mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w4.unmount();}}});c1[_2_index]=pvnode;w4.__owl__.pvnode = pvnode;});
+        } else {
+            def3 = def3 || w4.__updateProps(props4, extra.forceUpdate, w4.__owl__.isMounted ? extra.patchQueue : []);
+            def3 = def3.then(()=>{if (w4.__owl__.isDestroyed) {return};let pvnode=w4.__owl__.pvnode;pvnode.data.hook.insert = vn => {vn.elm.parentNode.replaceChild(w4.el,vn.elm);vn.elm=w4.el;w4.__remount();};c1[_2_index]=pvnode;});
+        }
+        extra.promises.push(def3);
+    }
+    return vn1;
+}"
+`;
+
 exports[`other directives with t-component t-on with handler bound to argument 1`] = `
 "function anonymous(context,extra
 ) {

tests/component/component.test.ts

@@ -733,15 +733,16 @@ describe("lifecycle hooks", () => {
     // we make sure here that willPatch/patched is only called if widget is in
     // dom, mounted
     const steps: string[] = [];
-    env.qweb.addTemplate(
+    env.qweb.addTemplates(`
-      "ParentWidget",
+        <templates>
-      `
+            <div t-name="ParentWidget">
-        <div>
+                <ChildWidget t-if="state.flag" v="state.n" t-keepalive="1"/>
-            <t t-if="state.flag" t-component="child" v="state.n" t-keepalive="1"/>
+            </div>
-        </div>`
+        </templates>
-    );
+    `);
+
     class ParentWidget extends Widget {
-      components = { child: ChildWidget };
+      components = { ChildWidget };
       state = { n: 1, flag: true };
     }
 
@@ -761,6 +762,8 @@ describe("lifecycle hooks", () => {
     }
     const widget = new ParentWidget(env);
     await widget.mount(fixture);
+
+    expect(env.qweb.templates.ParentWidget.fn.toString()).toMatchSnapshot();
     expect(steps).toEqual(["child:mounted"]);
     widget.state.flag = false;
     await nextTick();
@@ -1124,6 +1127,7 @@ describe("composition", () => {
     const input2 = fixture.getElementsByTagName("input")[0];
     expect(input).toBe(input2);
     expect(input2.value).toBe("test");
+    expect(env.qweb.templates.ParentWidget.fn.toString()).toMatchSnapshot();
   });
 
   test("sub widget with t-ref and t-keepalive", async () => {
@@ -4010,4 +4014,74 @@ describe("unmounting and remounting", () => {
     expect(fixture.innerHTML).toBe("<div>Hey</div>");
     expect(steps).toEqual(["willstart", "mounted"]);
   });
+
+  test("state changes in willUnmount do not trigger rerender", async () => {
+    const steps: string[] = [];
+    env.qweb.addTemplates(`
+        <templates>
+            <div t-name="Parent">
+                <Child t-if="state.flag" val="state.val"/>
+            </div>
+            <span t-name="Child"><t t-esc="props.val"/><t t-esc="state.n"/></span>
+        </templates>
+    `);
+
+    class Child extends Widget {
+      state = { n: 2 };
+      __render(a, b, c, d) {
+        steps.push("render");
+        return super.__render(a, b, c, d);
+      }
+      willPatch() {
+        steps.push("willPatch");
+      }
+      patched() {
+        steps.push("patched");
+      }
+
+      willUnmount() {
+        steps.push("willUnmount");
+        this.state.n = 3;
+      }
+    }
+    class Parent extends Widget {
+      components = { Child };
+      state = { val: 1, flag: true };
+    }
+
+    const widget = new Parent(env);
+    await widget.mount(fixture);
+    expect(steps).toEqual(["render"]);
+    expect(fixture.innerHTML).toBe("<div><span>12</span></div>");
+    widget.state.flag = false;
+    await nextTick();
+    // we make sure here that no call to __render is done
+    expect(steps).toEqual(["render", "willUnmount"]);
+  });
+
+  test("state changes in willUnmount will be applied on remount", async () => {
+    env.qweb.addTemplates(`
+        <templates>
+            <div t-name="TestWidget"><t t-esc="state.val"/></div>
+        </templates>
+    `);
+
+    class TestWidget extends Widget {
+      state = { val: 1 };
+      willUnmount() {
+        this.state.val = 3;
+      }
+    }
+
+    const widget = new TestWidget(env);
+    await widget.mount(fixture);
+    expect(fixture.innerHTML).toBe("<div>1</div>");
+    widget.unmount();
+    expect(fixture.innerHTML).toBe("");
+    await widget.mount(fixture);
+    expect(fixture.innerHTML).toBe("<div>1</div>");
+    widget.unmount();
+    await widget.mount(fixture, true);
+    expect(fixture.innerHTML).toBe("<div>3</div>");
+  });
 });