represent multi callback as a pair of callbacks + tests

src/runtime/reactivity.ts

@@ -1,4 +1,4 @@
-import type { Callback, SimpleCB } from "./utils";
+import type { Callback } from "./utils";
 import { OwlError } from "../common/owl_error";
 
 // Special key to subscribe to, to be notified of key creation/deletion
@@ -107,6 +107,19 @@ function observeTargetKey(target: Target, key: PropertyKey, callback: Callback):
   }
   callbacksToTargets.get(callback)!.add(target);
 }
+
+function clearAndCall(callback: Callback) {
+  clearReactivesForCallback(callback);
+  if (callback instanceof Array) {
+    // Recursively clear and call all callback pairs.
+    for (const cb of callback) {
+      clearAndCall(cb);
+    }
+  } else {
+    callback();
+  }
+}
+
 /**
  * Notify Reactives that are observing a given target that a key has changed on
  * the target.
@@ -126,19 +139,8 @@ function notifyReactives(target: Target, key: PropertyKey): void {
     return;
   }
   // Loop on copy because clearReactivesForCallback will modify the set in place
-  const allSimpleCbs = new Set<SimpleCB>();
   for (const callback of [...callbacks]) {
-    clearReactivesForCallback(callback);
-    if (callback instanceof Set) {
-      for (const cb of callback) {
-        allSimpleCbs.add(cb);
-      }
-    } else {
-      allSimpleCbs.add(callback);
-    }
-  }
-  for (const callback of allSimpleCbs) {
-    callback();
+    clearAndCall(callback);
   }
 }
 
@@ -251,22 +253,7 @@ export function multiReactive<T extends Target>(
 ): T {
   const existingCB = proxyToCallback.get(reactiveTarget);
   if (existingCB && existingCB !== NO_CALLBACK) {
-    const multiCB = new Set<SimpleCB>();
-    if (callback instanceof Set) {
-      for (const cb of callback) {
-        multiCB.add(cb);
-      }
-    } else {
-      multiCB.add(callback);
-    }
-    if (existingCB instanceof Set) {
-      for (const cb of existingCB) {
-        multiCB.add(cb);
-      }
-    } else {
-      multiCB.add(existingCB);
-    }
-    return reactive(reactiveTarget, multiCB);
+    return reactive(reactiveTarget, [callback, existingCB]);
   } else {
     return reactive(reactiveTarget, callback);
   }

src/runtime/utils.ts

@@ -1,7 +1,5 @@
 import { OwlError } from "../common/owl_error";
-export type Callback = SimpleCB | MultiCB;
-export type MultiCB = Set<SimpleCB>;
-export type SimpleCB = () => void;
+export type Callback = (() => void) | [first: Callback, second: Callback];
 
 /**
  * Creates a batched version of a callback so that all calls to it in the same

tests/computed.test.ts

@@ -384,4 +384,77 @@ describe("computed - with components", () => {
     // all will be re-computed and only once
     expectComputeCounts({ c: 1, d: 1, e: 1, f: 1 });
   });
+
+  test("more complicated compute tree", async () => {
+    const state = reactive({ x: 3, y: 2 });
+    const b = computed((self: typeof state) => {
+      return self.x + self.y;
+    }, "computed b");
+    const e = computed((self: typeof state) => {
+      return b(self) * self.x;
+    }, "computed e");
+    const f = computed((self: typeof state) => {
+      return e(self) + b(self);
+    }, "computed f");
+
+    const renderCounts = { A: 0, C: 0 };
+    const expectRenderCounts = (expected: { A: number; C: number }) => {
+      expect(renderCounts).toEqual(expected);
+      renderCounts.A = 0;
+      renderCounts.C = 0;
+    };
+
+    class BaseComp extends Component {
+      state = useState(state);
+      setup() {
+        Object.assign(this, { b, e, f });
+      }
+    }
+    class A extends BaseComp {
+      static components = {};
+      static template = xml`<p id="A" t-out="f(state)" />`;
+      setup() {
+        super.setup();
+        onRendered(() => {
+          renderCounts.A++;
+        });
+      }
+    }
+    class C extends BaseComp {
+      static components = {};
+      static template = xml`<p id="C" t-out="f(state)" />`;
+      setup() {
+        super.setup();
+        onRendered(() => {
+          renderCounts.C++;
+        });
+      }
+    }
+    class App extends Component {
+      static components = { A, C };
+      static template = xml`<div><A /><C /></div>`;
+    }
+
+    await mount(App, fixture);
+    expect(fixture.innerHTML).toEqual('<div><p id="A">20</p><p id="C">20</p></div>');
+    expectRenderCounts({ A: 1, C: 1 });
+
+    state.x = 4;
+    await nextTick();
+    expect(fixture.innerHTML).toEqual('<div><p id="A">30</p><p id="C">30</p></div>');
+    expectRenderCounts({ A: 1, C: 1 });
+
+    // Mutating both should also result to just one re-rendering.
+    state.x = 10;
+    state.y = 20;
+    await nextTick();
+    expect(fixture.innerHTML).toEqual('<div><p id="A">330</p><p id="C">330</p></div>');
+    expectRenderCounts({ A: 1, C: 1 });
+
+    // Setting without change of value should not re-render.
+    state.x = 10;
+    await nextTick();
+    expect(fixture.innerHTML).toEqual('<div><p id="A">330</p><p id="C">330</p></div>');
+    expectRenderCounts({ A: 0, C: 0 });
+  });
 });

tests/reactivity.test.ts

@@ -9,7 +9,7 @@ import {
   markRaw,
   toRaw,
 } from "../src";
-import { reactive, getSubscriptions } from "../src/runtime/reactivity";
+import { reactive, getSubscriptions, multiReactive } from "../src/runtime/reactivity";
 import { batched } from "../src/runtime/utils";
 import {
   makeDeferred,
@@ -2367,3 +2367,104 @@ describe("Reactivity: useState", () => {
     expect(fixture.innerHTML).toBe("<div><p><span>2b</span></p></div>");
   });
 });
+
+describe("multiReactive", () => {
+  /**
+   * A version of effect that combines with the reactive
+   * object so that both have the same dependencies.
+   */
+  function effectMulti<T extends object>(dep: T, fn: (o: T) => void) {
+    const r: any = multiReactive(dep, () => fn(r));
+    fn(r);
+  }
+
+  test("basic", async () => {
+    let count = 0;
+    const expectCount = (expected: number) => {
+      expect(count).toBe(expected);
+      count = 0;
+    };
+
+    const t = reactive({ a: 3, b: 2 }, () => count++);
+
+    let val = 0;
+    effectMulti(t, (t) => {
+      count++;
+      val = t.a + t.b;
+    });
+
+    expect(val).toBe(5);
+    // count is one initially because only the effect is called.
+    // This is the time that the callback of t starts subscribing to changes of t.
+    expectCount(1);
+
+    t.a = 4;
+    expect(val).toBe(6);
+    // count will be 2 because t's callback is called and the effect is called.
+    expectCount(2);
+  });
+
+  test("doesn't call the NO_CALLBACK function", async () => {
+    let count = 0;
+    const expectCount = (expected: number) => {
+      expect(count).toBe(expected);
+      count = 0;
+    };
+
+    // This target has the NO_CALLBACK function as reactive.
+    const t = reactive({ a: 3, b: 2 });
+
+    let val = 0;
+    effectMulti(t, (t) => {
+      count++;
+      val = t.a + t.b;
+    });
+
+    expect(val).toBe(5);
+    expectCount(1);
+
+    t.a = 4;
+    expect(val).toBe(6);
+    expectCount(1);
+
+    t.a = 4;
+    expect(val).toBe(6);
+    expectCount(0);
+  });
+
+  test("properly clear reactives from other observed keys when already called", async () => {
+    // This only works for batched effects because there is time to clear
+    // the already called callbacks before the effect is called.
+
+    let counts = { t: 0, mt: 0 };
+    function expectCounts(expected: { t: number; mt: number }) {
+      expect(counts).toEqual(expected);
+      counts = { t: 0, mt: 0 };
+    }
+
+    const t = reactive({ a: 3, b: 2 }, () => counts.t++);
+    const mt = multiReactive(t, () => counts.mt++);
+
+    let val = 0;
+    effectMulti(
+      mt,
+      batched((t) => {
+        val = t.a * t.b;
+      })
+    );
+
+    await nextMicroTick();
+    expect(val).toBe(6);
+    expectCounts({ t: 0, mt: 0 });
+
+    t.a = 4;
+    t.b = 3;
+    await nextMicroTick();
+    expect(val).toBe(12);
+    // Even if both a and b changed, the reactives are only called once.
+    // When `a` is set, the mt and t reactives are notified, and during this
+    // notification, these reactives' subscription to `b` is removed. So when
+    // `b` is set, there are no more callbacks to call.
+    expectCounts({ t: 1, mt: 1 });
+  });
+});