[FIX] component: hooks mounted/willUnmount called in correct order

The issue was with parent/child relationship.  It is not intuitive to
me, but the mounted hook should be called first on the children, and
then on the parent.  willUnmount hooks is the opposite.

closes #63

src/component.ts

@@ -219,28 +219,38 @@ export class Component<
     target.appendChild(this.el!);
 
     if (document.body.contains(target)) {
-      this._visitSubTree(w => {
-        if (!w.__owl__.isMounted && this.el!.contains(w.el)) {
-          w.__owl__.isMounted = true;
-          w.mounted();
-          return true;
-        }
-        return false;
-      });
+      this._callMounted();
+    }
+  }
+
+  _callMounted() {
+    const children = this.__owl__.children;
+    for (let id in children) {
+      const comp = children[id];
+      if (!comp.__owl__.isMounted && this.el!.contains(comp.el)) {
+        comp._callMounted();
+      }
+    }
+    this.__owl__.isMounted = true;
+    this.mounted();
+  }
+
+  _callWillUnmount() {
+    this.willUnmount();
+    this.__owl__.isMounted = false;
+    const children = this.__owl__.children;
+    for (let id in children) {
+      const comp = children[id];
+      if (comp.__owl__.isMounted) {
+        comp._callWillUnmount();
+      }
     }
   }
 
   unmount() {
-    if (this.el) {
-      this._visitSubTree(w => {
-        if (w.__owl__.isMounted) {
-          w.willUnmount();
-          w.__owl__.isMounted = false;
-          return true;
-        }
-        return false;
-      });
-      this.el.remove();
+    if (this.__owl__.isMounted) {
+      this._callWillUnmount();
+      this.el!.remove();
     }
   }
 
@@ -260,27 +270,34 @@ export class Component<
 
   destroy() {
     if (!this.__owl__.isDestroyed) {
-      for (let id in this.__owl__.children) {
-        this.__owl__.children[id].destroy();
+      const el = this.el;
+      this._destroy();
+      if (el) {
+        el.remove();
       }
-      if (this.__owl__.isMounted) {
-        this.willUnmount();
-      }
-      if (this.el) {
-        this.el.remove();
-        this.__owl__.isMounted = false;
-        delete this.__owl__.vnode;
-      }
-      if (this.__owl__.parent) {
-        let id = this.__owl__.id;
-        delete this.__owl__.parent.__owl__.children[id];
-        this.__owl__.parent = null;
-      }
-      this.clear();
-      this.__owl__.isDestroyed = true;
     }
   }
 
+  _destroy() {
+    const isMounted = this.__owl__.isMounted;
+    if (isMounted) {
+      this.willUnmount();
+      this.__owl__.isMounted = false;
+    }
+    const children = Object.values(this.__owl__.children);
+    for (let child of children) {
+      child._destroy();
+    }
+    if (this.__owl__.parent) {
+      let id = this.__owl__.id;
+      delete this.__owl__.parent.__owl__.children[id];
+      this.__owl__.parent = null;
+    }
+    this.clear();
+    this.__owl__.isDestroyed = true;
+    delete this.__owl__.vnode;
+  }
+
   shouldUpdate(nextProps: Props): boolean {
     return true;
   }
@@ -412,16 +429,6 @@ export class Component<
     }
   }
 
-  _visitSubTree(callback: (w: Component<T, any, any>) => boolean) {
-    const shouldVisitChildren = callback(this);
-    if (shouldVisitChildren) {
-      const children = this.__owl__.children;
-      for (let id in children) {
-        children[id]._visitSubTree(callback);
-      }
-    }
-  }
-
   _observeState() {
     if (this.state) {
       this.__owl__.observer = new Observer();

tests/component.test.ts

@@ -196,32 +196,29 @@ describe("lifecycle hooks", () => {
   });
 
   test("mounted hook is called on subwidgets, in proper order", async () => {
-    expect.assertions(4);
-    let parentMounted = false;
-    let childMounted = false;
+    const steps: any[] = [];
+
     class ParentWidget extends Widget {
       inlineTemplate = `<div><t t-widget="child"/></div>`;
       widgets = { child: ChildWidget };
       mounted() {
-        expect(childMounted).toBe(false);
-        parentMounted = true;
+        steps.push("parent:mounted");
       }
     }
     class ChildWidget extends Widget {
       mounted() {
         expect(document.body.contains(this.el)).toBe(true);
-        expect(parentMounted).toBe(true);
-        childMounted = true;
+        steps.push("child:mounted");
       }
     }
     const widget = new ParentWidget(env);
     await widget.mount(fixture);
-    expect(childMounted).toBe(true);
+    expect(steps).toEqual(["child:mounted", "parent:mounted"]);
   });
 
   test("willStart, mounted on subwidget rendered after main is mounted in some other position", async () => {
-    expect.assertions(3);
-    let hookCounter = 0;
+    const steps: string[] = [];
+
     // the t-else part in the template is important. This is
     // necessary to have a situation that could confuse the vdom
     // patching algorithm
@@ -240,19 +237,18 @@ describe("lifecycle hooks", () => {
     }
     class ChildWidget extends Widget {
       async willStart() {
-        hookCounter++;
+        steps.push("child:willStart");
       }
       mounted() {
-        expect(hookCounter).toBe(1);
-        hookCounter++;
+        steps.push("child:mounted");
       }
     }
     const widget = new ParentWidget(env);
     await widget.mount(fixture);
-    expect(hookCounter).toBe(0); // sub widget not created yet
+    expect(steps).toEqual([]);
     widget.state.ok = true;
     await nextTick();
-    expect(hookCounter).toBe(2);
+    expect(steps).toEqual(["child:willStart", "child:mounted"]);
   });
 
   test("mounted hook is correctly called on subwidgets created in mounted hook", async done => {
@@ -397,10 +393,10 @@ describe("lifecycle hooks", () => {
       "p willstart",
       "c init",
       "c willstart",
-      "p mounted",
       "c mounted",
-      "c willunmount",
-      "p willunmount"
+      "p mounted",
+      "p willunmount",
+      "c willunmount"
     ]);
   });
 
@@ -545,10 +541,10 @@ describe("lifecycle hooks", () => {
       state = { n: 1 };
       willPatch() {
         steps.push("parent:willPatch");
-        return 'leffe';
+        return "leffe";
       }
       patched(snapshot) {
-        expect(snapshot).toBe('leffe');
+        expect(snapshot).toBe("leffe");
         steps.push("parent:patched");
       }
     }
@@ -576,7 +572,6 @@ describe("lifecycle hooks", () => {
     ]);
   });
 
-
   test("willPatch/patched hook with t-keepalive", async () => {
     // we make sure here that willPatch/patched is only called if widget is in
     // dom, mounted
@@ -606,15 +601,18 @@ describe("lifecycle hooks", () => {
     }
     const widget = new ParentWidget(env);
     await widget.mount(fixture);
-    expect(steps).toEqual(['child:mounted']);
+    expect(steps).toEqual(["child:mounted"]);
     widget.state.flag = false;
     await nextTick();
-    expect(steps).toEqual(['child:mounted', 'child:willUnmount']);
+    expect(steps).toEqual(["child:mounted", "child:willUnmount"]);
     widget.state.flag = true;
     await nextTick();
-    expect(steps).toEqual(['child:mounted', 'child:willUnmount', 'child:mounted']);
+    expect(steps).toEqual([
+      "child:mounted",
+      "child:willUnmount",
+      "child:mounted"
+    ]);
   });
-
 });
 
 describe("destroy method", () => {
@@ -675,7 +673,7 @@ describe("destroy method", () => {
     expect(widget.__owl__.isStarted).toBe(false);
     expect(widget.__owl__.isMounted).toBe(false);
     expect(widget.__owl__.isDestroyed).toBe(true);
-    expect(widget.__owl__.vnode).toBe(null);
+    expect(widget.__owl__.vnode).toBe(undefined);
     expect(fixture.innerHTML).toBe("");
     expect(isRendered).toBe(false);
   });
@@ -707,7 +705,7 @@ describe("composition", () => {
           <t t-foreach="state.list" t-ref="'child'" t-widget="Widget"/>
         </div>`;
       widgets = { Widget };
-      state = {list: <any>[]};
+      state = { list: <any>[] };
       willPatch() {
         expect(this.refs.child).toBeUndefined();
       }
@@ -719,7 +717,7 @@ describe("composition", () => {
     const parent = new ParentWidget(env);
     await parent.mount(fixture);
     parent.state.list.push(1);
-    await nextTick()
+    await nextTick();
   });
 
   test("t-refs are bound at proper timing (2)", async () => {
@@ -763,9 +761,9 @@ describe("composition", () => {
     const parent = new ParentWidget(env);
     await parent.mount(fixture);
     parent.state.child2 = true;
-    await nextTick()
+    await nextTick();
     parent.state.child1 = false;
-    await nextTick()
+    await nextTick();
   });
 
   test("modifying a sub widget", async () => {