[FIX] store/connect: fix bugs with parent/child connected

There were big issues when we use parent/child connected widgets.

- children was rendered twice (and mapStoreToProps was called twice)
- if children was supposed to be destroyed, it was rendered once.

We solve them in this commit by waiting for parent widgets to be ready
before updating children.

closes #216

doc/store.md

@@ -10,6 +10,8 @@
   - [Actions](#actions)
   - [Getters](#getters)
   - [Connecting a Component](#connecting-a-component)
+  - [Semantics](#semantics)
+  - [Good Practices](#good-practices)
 
 ## Overview
 
@@ -207,3 +209,29 @@ The arguments of `connect` are:
 
 The `connect` function returns a sub class of the given `Component` which is
 connected to the `store`.
+
+### Semantics
+
+The `Store` and the `connect` function try to be smart and to optimize as much
+as possible the rendering and update process.  What is important to know is:
+
+- components are always updated in the order of their creation (so, parent
+  before children)
+- they are updated only if they are in the DOM
+- if a parent is asynchronous, the system will wait for it to complete its
+  update before updating other components.
+- in general, updates are not coordinated.  This is not a problem for synchronous
+  components, but if there are many asynchronous components, this could lead to
+  a situation where some part of the UI is updated and other parts of the UI is
+  not updated.
+
+### Good Practices
+
+- avoid asynchronous components as much as possible.  Asynchronous components
+  lead to situations where parts of the UI is not updated immediately.
+- do not be afraid to connect many components, parent or children if needed. For
+  example, a `MessageList` component could get a list of ids in its `mapStoreToProps` and a `Message` component could get the data of its own
+  message
+- since the `mapStoreToProps` function is called for each connected component,
+  for each state update, it is important to make sure that these functions are
+  as fast as possible.

src/store.ts

@@ -48,6 +48,7 @@ export class Store extends EventBus {
   observer: Observer;
   getters: { [name: string]: (payload?) => any };
   _gettersCache: { [name: string]: {} };
+  _updateId: number = 1;
 
   constructor(config: StoreConfig, options: StoreOption = {}) {
     super();
@@ -57,10 +58,7 @@ export class Store extends EventBus {
     this.mutations = config.mutations;
     this.env = config.env;
     this.observer = new Observer();
-    this.observer.notifyCB = () => {
+    this.observer.notifyCB = this.__notifyComponents.bind(this);
-      this._gettersCache = {};
-      this.trigger("update");
-    };
     this.observer.allowMutations = false;
     this.observer.observe(this.state);
     this.getters = {};
@@ -139,6 +137,34 @@ export class Store extends EventBus {
     this._commitLevel--;
     return res;
   }
+
+  /**
+   * Instead of using trigger to emit an update event, we actually implement
+   * our own function to do that.  The reason is that we need to be smarter than
+   * a simple trigger function: we need to wait for parent components to be
+   * done before doing children components.  The reason is that if an update
+   * as an effect of destroying a children, we do not want to call the
+   * mapStoreToProps function of the child, nor rendering it.
+   *
+   * This method is not optimal if we have a bunch of asynchronous components:
+   * we wait sequentially for each component to be completed before updating the
+   * next.  However, the only things that matters is that children are updated
+   * after their parents.  So, this could be optimized by being smarter, and
+   * updating all widgets concurrently, except for parents/children.
+   */
+  async __notifyComponents() {
+    this._updateId++;
+    const current = this._updateId;
+    this._gettersCache = {};
+    const subs = this.subscriptions.update || [];
+    for (let i = 0, iLen = subs.length; i < iLen; i++) {
+      const sub = subs[i];
+      const shouldCallback = sub.owner ? sub.owner.__owl__.isMounted : true;
+      if (shouldCallback) {
+        await sub.callback.call(sub.owner, current);
+      }
+    }
+  }
 }
 
 //------------------------------------------------------------------------------
@@ -231,7 +257,7 @@ export function connect<E extends EnvWithStore, P, S>(
      * if we use the mounted hook, this will be done in the reverse order.
      */
     __callMounted() {
-      (<any>this.__owl__).store.on("update", this, this._checkUpdate);
+      (<any>this.__owl__).store.on("update", this, this.__checkUpdate);
       super.__callMounted();
     }
     willUnmount() {
@@ -239,7 +265,10 @@ export function connect<E extends EnvWithStore, P, S>(
       super.willUnmount();
     }
 
-    _checkUpdate() {
+    async __checkUpdate(updateId) {
+      if (updateId === (<any>this.__owl__).currentUpdateId) {
+        return;
+      }
       const ownProps = (<any>this.__owl__).ownProps;
       const storeProps = mapStoreToProps(
         (<any>this.__owl__).store.state,
@@ -265,22 +294,24 @@ export function connect<E extends EnvWithStore, P, S>(
       }
       if (didChange) {
         (<any>this.__owl__).currentStoreProps = storeProps;
-        this.__updateProps(ownProps, false);
+        await this.__updateProps(ownProps, false);
       }
     }
     __updateProps(nextProps, forceUpdate, patchQueue?: any[]) {
-      if ((<any>this.__owl__).ownProps !== nextProps) {
+      const __owl__ = <any>this.__owl__;
-        (<any>this.__owl__).currentStoreProps = mapStoreToProps(
+      __owl__.currentUpdateId = __owl__.store._updateId;
-          (<any>this.__owl__).store.state,
+      if (__owl__.ownProps !== nextProps) {
+        __owl__.currentStoreProps = mapStoreToProps(
+          __owl__.store.state,
           nextProps,
-          (<any>this.__owl__).store.getters
+          __owl__.store.getters
         );
       }
-      (<any>this.__owl__).ownProps = nextProps;
+      __owl__.ownProps = nextProps;
       const mergedProps = Object.assign(
         {},
         nextProps,
-        (<any>this.__owl__).currentStoreProps
+        __owl__.currentStoreProps
       );
       return super.__updateProps(mergedProps, forceUpdate, patchQueue);
     }

tests/store.test.ts

@@ -6,6 +6,7 @@ import {
   nextMicroTick,
   nextTick
 } from "./helpers";
+import { Observer } from "../src";
 
 describe("basic use", () => {
   test("commit a mutation", () => {
@@ -207,7 +208,7 @@ describe("basic use", () => {
       bestBeerName({ state }) {
         n++;
         return state.beers[1].name;
-      },
+      }
     };
     const store = new Store({ state, mutations: {}, actions: {}, getters });
     expect((<any>store.getters).bestBeerName()).toBe("bertinchamps");
@@ -226,7 +227,7 @@ describe("basic use", () => {
           name: "bertinchamps",
           tasterID: 1
         }
-      },
+      }
     };
     let n = 0;
     const getters = {
@@ -265,7 +266,7 @@ describe("basic use", () => {
       }
     };
     const mutations = {
-      changeTaster({ state }, {beerID, tasterID}) {
+      changeTaster({ state }, { beerID, tasterID }) {
         state.beers[beerID].tasterID = tasterID;
       }
     };
@@ -274,15 +275,20 @@ describe("basic use", () => {
       beerTasterName({ state }, beerID) {
         n++;
         return state.tasters[state.beers[beerID].tasterID].name;
-      },
+      }
     };
-    const store = new Store({ state, mutations: mutations, actions: {}, getters });
+    const store = new Store({
+      state,
+      mutations: mutations,
+      actions: {},
+      getters
+    });
     expect((<any>store.getters).beerTasterName(1)).toBe("aaron");
     expect(n).toBe(1);
     expect((<any>store.getters).beerTasterName(1)).toBe("aaron");
     expect(n).toBe(1);
 
-    store.commit('changeTaster', {beerID: 1, tasterID: 2});
+    store.commit("changeTaster", { beerID: 1, tasterID: 2 });
     await nextTick();
 
     expect((<any>store.getters).beerTasterName(1)).toBe("gery");
@@ -295,14 +301,14 @@ describe("basic use", () => {
         1: {
           id: 1,
           name: "bertinchamps"
-        },
+        }
       }
     };
     const mutations = {
       renameBeer({ state, getters }, beerID) {
-        expect(getters.beerName(beerID)).toBe('bertinchamps');
+        expect(getters.beerName(beerID)).toBe("bertinchamps");
-        state.beers[1].name = 'chouffe';
+        state.beers[1].name = "chouffe";
-        expect(getters.beerName(beerID)).toBe('chouffe');
+        expect(getters.beerName(beerID)).toBe("chouffe");
       }
     };
     let n = 0;
@@ -310,12 +316,17 @@ describe("basic use", () => {
       beerName({ state }, beerID) {
         n++;
         return state.beers[beerID].name;
-      },
+      }
     };
-    const store = new Store({ state, mutations: mutations, actions: {}, getters });
+    const store = new Store({
+      state,
+      mutations: mutations,
+      actions: {},
+      getters
+    });
 
-    store.commit('renameBeer', 1);
+    store.commit("renameBeer", 1);
-    expect((<any>store.getters).beerName(1)).toBe('chouffe');
+    expect((<any>store.getters).beerName(1)).toBe("chouffe");
     await nextTick();
 
     expect(n).toBe(3);
@@ -1117,7 +1128,190 @@ describe("connecting a component to store", () => {
 
     store.commit("setCurrent", "b");
     await nextTick();
-    expect(steps).toEqual(["parent", "child", "parent", "child", "child"]);
+    expect(fixture.innerHTML).toBe("<div><span>b</span></div>");
+    expect(steps).toEqual(["parent", "child", "parent", "child"]);
+  });
+
+  test("connected parent/children: no double rendering", async () => {
+    const mutations = {
+      editTodo({ state }) {
+        state.todos[1].title = "abc";
+      }
+    };
+    const todos = { 1: { id: 1, title: "kikoou" } };
+    const state = {
+      todos
+    };
+    const store = new Store({
+      state,
+      mutations
+    });
+
+    env.qweb.addTemplates(`
+        <templates>
+            <div t-name="TodoApp" class="todoapp">
+                <t t-foreach="Object.values(props.todos)" t-as="todo">
+                    <ConnectedTodoItem t-key="todo.id" id="todo.id"/>
+                </t>
+            </div>
+
+            <div t-name="TodoItem" class="todo">
+                <t t-esc="props.todo.title"/>
+                <button class="destroy" t-on-click="editTodo">x</button>
+            </div>
+        </templates>
+    `);
+
+    function mapStoreToPropsTodoApp(state) {
+      return {
+        todos: state.todos
+      };
+    }
+
+    class TodoApp extends Component<any, any, any> {
+      components = { ConnectedTodoItem };
+    }
+
+    const ConnectedTodoApp = connect(
+      TodoApp,
+      mapStoreToPropsTodoApp
+    );
+
+    let renderCount = 0;
+    let fCount = 0;
+
+    function mapStoreToPropsTodoItem(state, ownProps) {
+      fCount++;
+      return {
+        todo: state.todos[ownProps.id]
+      };
+    }
+
+    class TodoItem extends Component<any, any, any> {
+      state = { isEditing: false };
+
+      editTodo() {
+        this.env.store.commit("editTodo");
+      }
+      __render(...args) {
+        renderCount++;
+        return super.__render(...args);
+      }
+    }
+
+    const ConnectedTodoItem = connect(
+      TodoItem,
+      mapStoreToPropsTodoItem
+    );
+
+    (<any>env).store = store;
+    const app = new ConnectedTodoApp(env);
+
+    await app.mount(fixture);
+    expect(fixture.innerHTML).toBe(
+      '<div class="todoapp"><div class="todo">kikoou<button class="destroy">x</button></div></div>'
+    );
+
+    expect(renderCount).toBe(1);
+    expect(fCount).toBe(1);
+    fixture.querySelector("button")!.click();
+    await nextTick();
+    expect(renderCount).toBe(2);
+    expect(fCount).toBe(2);
+    expect(fixture.innerHTML).toBe(
+      '<div class="todoapp"><div class="todo">abc<button class="destroy">x</button></div></div>'
+    );
+  });
+
+  test("connected parent/children: no rendering if child is destroyed", async () => {
+    const mutations = {
+      removeTodo({ state }) {
+        Observer.delete(state.todos, 1);
+      }
+    };
+    const todos = { 1: { id: 1, title: "kikoou" } };
+    const state = {
+      todos
+    };
+    const store = new Store({
+      state,
+      mutations
+    });
+
+    env.qweb.addTemplates(`
+        <templates>
+            <div t-name="TodoApp" class="todoapp">
+                <t t-foreach="Object.values(props.todos)" t-as="todo">
+                    <ConnectedTodoItem t-key="todo.id" id="todo.id"/>
+                </t>
+            </div>
+
+            <div t-name="TodoItem" class="todo">
+                <t t-esc="props.todo.title"/>
+                <button class="destroy" t-on-click="removeTodo">x</button>
+            </div>
+        </templates>
+    `);
+
+    function mapStoreToPropsTodoApp(state) {
+      return {
+        todos: state.todos
+      };
+    }
+
+    class TodoApp extends Component<any, any, any> {
+      components = { ConnectedTodoItem };
+    }
+
+    const ConnectedTodoApp = connect(
+      TodoApp,
+      mapStoreToPropsTodoApp
+    );
+
+    let renderCount = 0;
+    let fCount = 0;
+
+    function mapStoreToPropsTodoItem(state, ownProps) {
+      fCount++;
+      return {
+        todo: state.todos[ownProps.id]
+      };
+    }
+
+    class TodoItem extends Component<any, any, any> {
+      state = { isEditing: false };
+
+      removeTodo() {
+        this.env.store.commit("removeTodo");
+      }
+      __render(...args) {
+        renderCount++;
+        return super.__render(...args);
+      }
+    }
+
+    const ConnectedTodoItem = connect(
+      TodoItem,
+      mapStoreToPropsTodoItem
+    );
+
+    (<any>env).store = store;
+    const app = new ConnectedTodoApp(env);
+
+    await app.mount(fixture);
+    expect(fixture.innerHTML).toBe(
+      '<div class="todoapp"><div class="todo">kikoou<button class="destroy">x</button></div></div>'
+    );
+
+    expect(renderCount).toBe(1);
+    expect(fCount).toBe(1);
+    fixture.querySelector("button")!.click();
+    await nextTick();
+    expect(renderCount).toBe(1);
+    expect(fCount).toBe(1);
+    expect(fixture.innerHTML).toBe(
+      '<div class="todoapp"></div>'
+    );
   });
 
   test("connected component willpatch/patch hooks are called on store updates", async () => {
@@ -1159,19 +1353,28 @@ describe("connecting a component to store", () => {
   });
 
   test("connected component has its own name", () => {
-    function mapStoreToProps() { }
+    function mapStoreToProps() {}
 
-    class Named extends Component<any, any, any> { };
+    class Named extends Component<any, any, any> {}
-    const namedConnected = connect(Named, mapStoreToProps);
+    const namedConnected = connect(
-    expect(namedConnected.name).toMatch('ConnectedNamed');
+      Named,
+      mapStoreToProps
+    );
+    expect(namedConnected.name).toMatch("ConnectedNamed");
 
-    class ParentNamed extends Component<any, any, any>{};
+    class ParentNamed extends Component<any, any, any> {}
-    class ChildNamed extends ParentNamed{};
+    class ChildNamed extends ParentNamed {}
-    const childConnected = connect(ChildNamed, mapStoreToProps)
+    const childConnected = connect(
-    expect(childConnected.name).toMatch('ConnectedChildNamed')
+      ChildNamed,
+      mapStoreToProps
+    );
+    expect(childConnected.name).toMatch("ConnectedChildNamed");
 
-    const Anonymous = class extends Component<any, any, any>{ };
+    const Anonymous = class extends Component<any, any, any> {};
-    const anonymousConnected = connect(Anonymous, mapStoreToProps);
+    const anonymousConnected = connect(
+      Anonymous,
+      mapStoreToProps
+    );
     expect(anonymousConnected.name).toMatch(/^Connectedclass_\d+/);
   });
 });