[FIX] ConnectedComponent: keep props uptodate

This is a tricky bug. The problem is that a ConnectedComponent can
trigger a rendering before it is aware that there is a state change in
the store, and just before the store update event comes in.

What we do in this commit is to update the storeProps whenever a
rendering is scheduled.  However, we need to keep the rendering
information as a promise to give it in some case to the __checkUpdate
method (see the note in the render method)

closes #268

src/store/connected_component.ts

@@ -56,6 +56,7 @@ export class ConnectedComponent<T extends Env, P, S> extends Component<T, P, S>
     const observer = store.observer;
     const revFn = this.deep ? observer.deepRevNumber : observer.revNumber;
     (this.__owl__ as any).store = store;
+    (this.__owl__ as any).ownProps = this.props;
     (this.__owl__ as any).revFn = revFn.bind(observer);
     (this.__owl__ as any).storeHash = this.hashFunction(this.storeProps, {
       prevStoreProps: this.storeProps
@@ -81,13 +82,44 @@ export class ConnectedComponent<T extends Env, P, S> extends Component<T, P, S>
     super.__destroy(parent);
   }
 
+  async render(force: boolean = false) {
+    this.__updateStoreProps(this.props);
+
+    // this is quite technical, so this deserves some explanation.
+    // When we have a connected component, it can be updated for 3 reasons:
+    // - some internal state changes (this will go through this method)
+    // - some props changes (if a parent is changed and need to rerender itself)
+    // - a store update
+    //
+    // It is possible (with connected component and parent) to have the following
+    // situation: the parent component is rendered first (from its state change),
+    // then immediately after, it is rendered (from store update). Then, if the
+    // __checkUpdate method is immediately over, the children component will
+    // be rendered again by the store update, even though it is supposed to be
+    // destroyed by the first rendering.
+    //
+    // So, the solution is to keep the information that there is a current
+    // rendering occuring with the same store state, the same props, and return
+    // that in the __checkUpdate method.  To do this, we use the renderPromise
+    // deferred, which is not used by the component system once the
+    // component is ready, so we can use it for our own purpose.
+    (this.__owl__ as any).renderPromise = super.render(force);
+    return (this.__owl__ as any).renderPromise;
+  }
+
   async __updateProps(nextProps: P, f, p, s, v) {
     this.__updateStoreProps(nextProps);
     return super.__updateProps(nextProps, f, p, s, v);
   }
 
   __updateStoreProps(nextProps): boolean {
-    const store = (this.__owl__ as any).store;
+    const __owl__ = this.__owl__ as any;
+    const store = __owl__.store;
+    const observer = store.observer;
+    if (observer.rev === __owl__.rev && nextProps === __owl__.ownProps) {
+      return false;
+    }
+
     const storeProps = (<any>this.constructor).mapStoreToProps(
       store.state,
       nextProps,
@@ -97,23 +129,21 @@ export class ConnectedComponent<T extends Env, P, S> extends Component<T, P, S>
     const storeHash = this.hashFunction(storeProps, options);
     this.storeProps = storeProps;
     let didChange = options.didChange;
-    if (storeHash !== (this.__owl__ as any).storeHash) {
+    if (storeHash !== __owl__.storeHash) {
-      (this.__owl__ as any).storeHash = storeHash;
+      __owl__.storeHash = storeHash;
       didChange = true;
     }
-    (this.__owl__ as any).rev = store.observer.rev;
+    __owl__.rev = store.observer.rev;
+    __owl__.ownProps = nextProps;
     return didChange;
   }
 
   async __checkUpdate() {
-    const observer = (this.__owl__ as any).store.observer;
-    if (observer.rev === (this.__owl__ as any).rev) {
-      // update was already done by updateProps, from parent
-      return;
-    }
     const didChange = this.__updateStoreProps(this.props);
     if (didChange) {
       return this.render();
     }
+    // see note in render method
+    return (this.__owl__ as any).renderPromise;
   }
 }

tests/store/__snapshots__/connected_component.test.ts.snap

@@ -15,3 +15,7 @@ exports[`connecting a component to store deep and shallow connecting a component
 exports[`connecting a component to store deep and shallow connecting a component 3`] = `"<div><span>Bertinchamps</span></div>"`;
 
 exports[`connecting a component to store deep and shallow connecting a component 4`] = `"<div><span>Kasteel</span></div>"`;
+
+exports[`various scenarios scenarios with async store updates and some components events 1`] = `"<div><button>Do stuff</button><div><span>Attachment 100</span><span>Name: text.txt</span></div></div>"`;
+
+exports[`various scenarios scenarios with async store updates and some components events 2`] = `"<div><button>Do stuff</button></div>"`;

tests/store/connected_component.test.ts

@@ -1094,3 +1094,85 @@ describe("connected components and default values", () => {
     expect(fixture.innerHTML).toBe("<div>1</div>");
   });
 });
+
+describe("various scenarios", () => {
+  let fixture: HTMLElement;
+  let env: Env;
+
+  beforeEach(() => {
+    fixture = makeTestFixture();
+    env = makeTestEnv();
+  });
+
+  afterEach(() => {
+    fixture.remove();
+  });
+
+  test("scenarios with async store updates and some components events", async () => {
+    const actions = {
+      async deleteAttachment({ state }) {
+        await Promise.resolve();
+        delete state.attachments[100];
+        state.messages[10].attachmentIds = [];
+      }
+    };
+    const state = {
+      attachments: {
+        100: {
+          id: 100,
+          name: "text.txt"
+        }
+      },
+      messages: {
+        10: {
+          attachmentIds: [100],
+          id: 10
+        }
+      }
+    };
+    const store = new Store({ actions, state });
+
+    env.qweb.addTemplates(`
+        <templates>
+            <div t-name="Message">
+                <button t-on-click="doStuff">Do stuff</button>
+                <Attachment t-foreach="storeProps.attachmentIds" t-key="attachmentId" t-as="attachmentId" id="attachmentId"/>
+            </div>
+            <div t-name="Attachment">
+                <span>Attachment <t t-esc="props.id"/></span>
+                <span>Name: <t t-esc="storeProps.name"/></span>
+            </div>
+        </templates>
+    `);
+    class Attachment extends ConnectedComponent<any, any, any> {
+      static mapStoreToProps(state, ownProps) {
+        return {
+          name: state.attachments[ownProps.id].name
+        };
+      }
+    }
+    class Message extends ConnectedComponent<any, any, any> {
+      static mapStoreToProps(state) {
+        return {
+          attachmentIds: state.messages[10].attachmentIds
+        };
+      }
+      components = { Attachment };
+      state = { isAttachmentDeleted: false };
+      doStuff() {
+        this.dispatch("deleteAttachment", 100);
+        this.state.isAttachmentDeleted = true;
+      }
+    }
+
+    (<any>env).store = store;
+    const message = new Message(env);
+    await message.mount(fixture);
+
+    expect(fixture.innerHTML).toMatchSnapshot();
+
+    fixture.querySelector("button")!.click();
+    await nextTick();
+    expect(fixture.innerHTML).toMatchSnapshot();
+  });
+});