From 4a96eff3c66780db1773cfb9508e47c285ea963a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= <ged@odoo.com>
Date: Thu, 7 Jan 2021 12:36:46 +0100
Subject: [PATCH] [FIX] store: properly call onUpdate functions in some cases

Before this commit, the following scenario could happen:

Suppose that we have a parent component A, connected to a store,
 and a child component B, also connected to the store and using
the onUpdate feature.

Then, we remount the A component in some other places and a
rendering is initiated in A.  We immediately update the store state.
What happens next is:

- rendering A is done, A internal revid is updated
- store update A (but nothing is done because the state change here
  does not modify A)
- rendering B is done (from parent), B internal revid is updated
- store update B, notice internal revid is updated, does not call the
  onUpdate function

We then have the B component which has not its internal state updated,
because we did not call its onUpdate function.

The solution is to move the onUpdate call in a "preupdate" event, to be
sure that it is called everytime the store is updated.

closes #816
---
 src/store.ts              | 17 ++++++++++--
 tests/store_hooks.test.ts | 58 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/src/store.ts b/src/store.ts
index ead4a763..ed2adcc9 100644
--- a/src/store.ts
+++ b/src/store.ts
@@ -75,6 +75,11 @@ export class Store extends Context {
     );
     return result;
   }
+
+  __notifyComponents(): Promise<void> {
+    this.trigger("before-update");
+    return super.__notifyComponents();
+  }
 }
 
 interface SelectorOptions {
@@ -105,13 +110,16 @@ export function useStore(selector, options: SelectorOptions = {}): any {
     const newRevNumber = hashFn(result);
     if ((newRevNumber > 0 && revNumber !== newRevNumber) || !isEqual(oldResult, result)) {
       revNumber = newRevNumber;
-      if (options.onUpdate) {
-        options.onUpdate(result);
-      }
       return true;
     }
     return false;
   }
+  if (options.onUpdate) {
+    store.on("before-update", component, () => {
+      const newValue = selector(store!.state, component.props!);
+      options.onUpdate(newValue);
+    });
+  }
   store.updateFunctions[componentId].push(function (): boolean {
     return selectCompareUpdate(store!.state, component.props);
   });
@@ -132,6 +140,9 @@ export function useStore(selector, options: SelectorOptions = {}): any {
   const __destroy = component.__destroy;
   component.__destroy = (parent) => {
     delete store.updateFunctions[componentId];
+    if (options.onUpdate) {
+      store.off("before-update", component);
+    }
     __destroy.call(component, parent);
   };
 
diff --git a/tests/store_hooks.test.ts b/tests/store_hooks.test.ts
index 3a54d540..2ecc0548 100644
--- a/tests/store_hooks.test.ts
+++ b/tests/store_hooks.test.ts
@@ -571,12 +571,12 @@ describe("connecting a component to store", () => {
     app.state.beerId = 2;
     await nextTick();
     expect(fixture.innerHTML).toBe("<div><span>kwak</span></div>");
-    expect(counter).toBe(1);
+    expect(counter).toBe(0);
 
     store.dispatch("renameBeer", { id: 2, name: "orval" });
     await nextTick();
     expect(fixture.innerHTML).toBe("<div><span>orval</span></div>");
-    expect(counter).toBe(2);
+    expect(counter).toBe(1);
   });
 
   test("connected component is properly cleaned up on destroy", async () => {
@@ -1345,4 +1345,58 @@ describe("various scenarios", () => {
     await nextTick();
     expect(fixture.innerHTML).toBe("<div><div>testWorld<div>3</div></div></div>");
   });
+
+  test("parent/children with store, parent is remounted", async () => {
+    const store = new Store({ state: { a: 1, b: 1 } });
+
+    class Child extends Component {
+      static template = xml`<div><t t-esc="a"/></div>`;
+      a: any;
+      constructor(parent, props) {
+        super(parent, props);
+        this.a = useStore(
+          (state, props) => {
+            return state.a;
+          },
+          {
+            onUpdate: (a) => {
+              this.a = a;
+            },
+          }
+        );
+      }
+    }
+
+    class Parent extends Component {
+      static template = xml`
+        <div>
+          parent: <t t-esc="b"/>
+          <Child/>
+        </div>`;
+      static components = { Child };
+
+      b: any;
+      constructor(parent, props) {
+        super(parent, props);
+        this.b = useStore((state, props) => {
+          return state.b;
+        });
+      }
+    }
+    (env as any).store = store;
+
+    const div = document.createElement("div");
+    fixture.appendChild(div);
+
+    // initial mounting
+    const parent = await mount(Parent, { target: fixture, env });
+    expect(fixture.innerHTML).toBe("<div></div><div> parent: 1<div>1</div></div>");
+
+    // remounting component, then immediately update store.state
+    parent.mount(div);
+    store.state.a++;
+
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<div><div> parent: 1<div>2</div></div></div>");
+  });
 });