From a3d563ebb4d9cd9b724927ce5952d33ad704da58 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= <ged@odoo.com>
Date: Tue, 27 Aug 2019 16:50:50 +0200
Subject: [PATCH] [FIX] connected_component: some misc issues

- did not properly stop listening to the store after being destroyed
- did not properly synchronize store updates in some async cases
---
 src/store/connected_component.ts        |   6 +-
 tests/store/connected_component.test.ts | 107 +++++++++++++++++++++++-
 2 files changed, 111 insertions(+), 2 deletions(-)

diff --git a/src/store/connected_component.ts b/src/store/connected_component.ts
index 7064104e..8b4a2e30 100644
--- a/src/store/connected_component.ts
+++ b/src/store/connected_component.ts
@@ -77,6 +77,10 @@ export class ConnectedComponent<T extends Env, P, S> extends Component<T, P, S>
     (this.__owl__ as any).store.off("update", this);
     super.__callWillUnmount();
   }
+  __destroy(parent: any) {
+    (this.__owl__ as any).store.off("update", this);
+    super.__destroy(parent);
+  }
 
   async __updateProps(nextProps: P, f, p, s, v) {
     this.__updateStoreProps(nextProps);
@@ -110,7 +114,7 @@ export class ConnectedComponent<T extends Env, P, S> extends Component<T, P, S>
     }
     const didChange = this.__updateStoreProps(this.props);
     if (didChange) {
-      this.render();
+      return this.render();
     }
   }
 }
diff --git a/tests/store/connected_component.test.ts b/tests/store/connected_component.test.ts
index 7039f501..3ff81f89 100644
--- a/tests/store/connected_component.test.ts
+++ b/tests/store/connected_component.test.ts
@@ -1,7 +1,7 @@
 import { Component, Env } from "../../src/component/component";
 import { ConnectedComponent } from "../../src/store/connected_component";
 import { Store } from "../../src/store/store";
-import { makeTestEnv, makeTestFixture, nextTick } from "../helpers";
+import { makeTestEnv, makeTestFixture, nextTick, makeDeferred } from "../helpers";
 
 describe("connecting a component to store", () => {
   let fixture: HTMLElement;
@@ -595,6 +595,66 @@ describe("connecting a component to store", () => {
     expect(steps).toEqual(["parent", "child", "parent", "child"]);
   });
 
+  test("correct update order when parent/children are connectedddd", async () => {
+    const steps: string[] = [];
+    let def = makeDeferred();
+    def.resolve();
+
+    env.qweb.addTemplates(`
+        <templates>
+            <div t-name="Parent">
+                <Child t-if="storeProps.flag" someId="storeProps.someId"/>
+            </div>
+            <span t-name="Child"><t t-esc="storeProps.msg"/></span>
+        </templates>
+    `);
+
+    class Parent extends ConnectedComponent<any, any, any> {
+      components = { Child };
+      static mapStoreToProps(s) {
+        steps.push("parent");
+        return { flag: s.flag, someId: s.someId };
+      }
+      async render(force) {
+          await def;
+          return super.render(force);
+      }
+    }
+
+    class Child extends ConnectedComponent<any, any, any> {
+      static mapStoreToProps(s, props) {
+        steps.push("child");
+        return { msg: s.messages[props.someId] };
+      }
+    }
+
+    const state = { someId: 1, flag: true, messages: {1: "abc"}};
+    const actions = {
+      setFlagToFalse({ state }) {
+        state.flag = false;
+      }
+    };
+
+    const store = new Store({ state, actions });
+    (<any>env).store = store;
+    const app = new Parent(env);
+
+    await app.mount(fixture);
+    expect(fixture.innerHTML).toBe("<div><span>abc</span></div>");
+    expect(steps).toEqual(["parent", "child"]);
+
+    def = makeDeferred();
+    store.dispatch("setFlagToFalse");
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<div><span>abc</span></div>");
+    expect(steps).toEqual(["parent", "child", "parent"]);
+
+    def.resolve();
+    await nextTick();
+    expect(steps).toEqual(["parent", "child", "parent"]);
+    expect(fixture.innerHTML).toBe("<div></div>");
+  });
+
   test("connected parent/children: no double rendering", async () => {
     const actions = {
       editTodo({ state }) {
@@ -948,4 +1008,49 @@ describe("connected components and default values", () => {
     await nextTick();
     expect(fixture.innerHTML).toBe("<div><div><div>200UpdatedMessage200</div></div></div>");
   });
+
+  test("connected child components stop listening to store when destroyed", async () => {
+    let steps: any = [];
+    env.qweb.addTemplates(`
+        <templates>
+          <div t-name="Parent">
+              <Child t-if="state.child" />
+          </div>
+          <div t-name="Child"><t t-esc="storeProps.val"/></div>
+        </templates>
+    `);
+    class Child extends ConnectedComponent<any, any, any> {
+      static mapStoreToProps(s) {
+        return s;
+      }
+    }
+
+    class Parent extends Component<any, any, any> {
+      components = { Child };
+      state = { child: true };
+    }
+
+    class TestStore extends Store {
+      on(eventType, owner, callback) {
+        steps.push(`on:${eventType}`);
+        super.on(eventType, owner, callback);
+      }
+      off(eventType, owner) {
+        steps.push(`off:${eventType}`);
+        super.off(eventType, owner);
+      }
+    }
+    const store = new TestStore({ state: {val: 1} });
+    (<any>env).store = store;
+    const parent = new Parent(env);
+
+    await parent.mount(fixture);
+    expect(steps).toEqual(["on:update"]);
+    expect(fixture.innerHTML).toBe("<div><div>1</div></div>");
+
+    parent.state.child = false;
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<div></div>");
+    expect(steps).toEqual(["on:update", "off:update"]);
+  });
 });