From 51b2ea07551b6368748e7f0d661297877c1b531d Mon Sep 17 00:00:00 2001
From: Joseph Caburnay <caburnay.joseph@gmail.com>
Date: Wed, 19 Jul 2023 00:09:40 +0200
Subject: [PATCH 1/3] [IMP] lazy reactive computed value

**Problem:** Prior to this commit, existing implementations of computed values
are eager. An example is the `withComputedProperties` in odoo's web addon, see:
[^1]. It has an issue such that it can lead to triggering of the compute
function even if the target is still in invalid state -- e.g. removing an item
in an array. This can be solved by batching the recomputation with `batched`.
But once the compute is batched, the computed value can't be used by other
computed values, because it won't recompute until the next tick -- it's not part
of the same synchronous context.

**Proposed solution:** This commit introduces a implementation of computed value
that is pull-based -- it only recomputes when it's needed. The main logic is the
following:

- A computed value is declared with a single-parameter function wrapped with
  `computed`.
- The compute function has a cache that stores the result of the computation.
- The cached value is invalidated when one of the dependencies of the compute
  function changes.
  - During invalidation, the cache is only marked as invalid and the
    recomputation is not made.
- A dependent computation such as an effect may indirectly depend in the
  dependencies of a computed value[^2]. This is an important detail because
  without this, the 'laziness' of the computed value won't trigger the dependent
  effect.
  - This indirect dependency tracking is achieved by using the introduced notion
    of multi-reactive target where a target can have multiple callbacks linked
    to it.
  - During recomputation of the computed value, both the target's reactive and
    the invalidation callback are subscribed to the dependencies of the computed
    value. But when the value is cached, only the target's reactive callback is
    subscribed.

With this implementation, we achieve a lazy computed value that plays well with
the existing reactive system.

[^1]: https://github.com/odoo/odoo/blob/15d7cf12b98bf0223371b8cca91ef6f79c60ee31/addons/web/static/src/core/utils/reactive.js#L58
[^2]: It's possible that in the dependency graph, an effect is affected by
invalidation. This effect will be triggered which will potentially ask for the
value of the computed value which will trigger the recomputation.
---
 src/runtime/computed.ts   |  79 +++++++
 src/runtime/index.ts      |   1 +
 src/runtime/reactivity.ts |  38 +++-
 src/runtime/utils.ts      |   6 +-
 tests/computed.test.ts    | 460 ++++++++++++++++++++++++++++++++++++++
 tests/reactivity.test.ts  | 103 ++++++++-
 6 files changed, 679 insertions(+), 8 deletions(-)
 create mode 100644 src/runtime/computed.ts
 create mode 100644 tests/computed.test.ts

diff --git a/src/runtime/computed.ts b/src/runtime/computed.ts
new file mode 100644
index 000000000..4815ccfdd
--- /dev/null
+++ b/src/runtime/computed.ts
@@ -0,0 +1,79 @@
+import { Reactive, Target, multiReactive, toRaw } from "./reactivity";
+
+/**
+ * Creates a lazy reactive computed value.
+ *
+ * Calling the resulting function on the target not only returns the computed value,
+ * it also caches the result in the target. As a result, succeeding function calls
+ * will not trigger recalculation. And because of the reactivity system, the cached
+ * value will be invalidated when any of the dependencies of the compute function
+ * changes.
+ *
+ * Aside from caching, the computation is part of the reactivity system. This means
+ * that it plays well with rerendering. For example, having the following tree,
+ * `<Root><A/><B/></Root>`, where `A` reads from a computed value, when the computed
+ * value changes (or the dependencies of the computed value changes), only the
+ * components that read from the computed value will rerender. In this case, only
+ * `A` will rerender.
+ *
+ * Note that this is only valid for one target and one compute function.
+ * Use `computed` for shared compute functions.
+ */
+export function defineComputed(compute: (target: any) => any, name?: string) {
+  // This is the key that will be used to store the compute value in the target.
+  const cacheKey = name ? Symbol(name) : Symbol();
+  let isValid = false;
+  const invalidate = () => (isValid = false);
+  return (target: any) => {
+    if (isValid) {
+      // Return the cached value if it is still valid.
+      // This will subscribe the target's reactive directly to the cached value.
+      return target[cacheKey];
+    } else {
+      // Create a target with multiple reactives.
+      // - First is the original target's reactive.
+      // - Second is the invalidate function.
+      // This means that when any of the dependencies of the compute function changes,
+      // the invalidate function and the original target's reactive will be notified.
+      const mTarget = multiReactive(target, invalidate);
+      // Call the compute function on the multi-reactive target.
+      // This will subscribe the reactives to the dependencies of the compute function.
+      const value = compute(mTarget);
+      isValid = true;
+      try {
+        return value;
+      } finally {
+        // Right after return, the value is cached in the target.
+        // This will notify the subscribers of this computed value.
+        target[cacheKey] = value;
+      }
+    }
+  };
+}
+
+// map: target -> compute -> cached compute
+const t2c2cc = new WeakMap();
+
+/**
+ * This allows sharing of a declared computed such that for each target-compute
+ * combination, there is a corresponding cached computed function.
+ */
+export function computed<T extends Target, R>(
+  compute: (target: T | Reactive<T>) => R,
+  name?: string
+) {
+  return (target: T | Reactive<T>): R => {
+    const raw = toRaw(target);
+    let c2cc = t2c2cc.get(raw);
+    if (!c2cc) {
+      c2cc = new Map();
+      t2c2cc.set(raw, c2cc);
+    }
+    let cachedCompute = c2cc.get(compute);
+    if (!cachedCompute) {
+      cachedCompute = defineComputed(compute, name);
+      c2cc.set(compute, cachedCompute);
+    }
+    return cachedCompute(target);
+  };
+}
diff --git a/src/runtime/index.ts b/src/runtime/index.ts
index 651263a9c..f0eb23637 100644
--- a/src/runtime/index.ts
+++ b/src/runtime/index.ts
@@ -40,6 +40,7 @@ export type { ComponentConstructor } from "./component";
 export { useComponent, useState } from "./component_node";
 export { status } from "./status";
 export { reactive, markRaw, toRaw } from "./reactivity";
+export { computed } from "./computed";
 export { useEffect, useEnv, useExternalListener, useRef, useChildSubEnv, useSubEnv } from "./hooks";
 export { EventBus, whenReady, loadFile, markup } from "./utils";
 export {
diff --git a/src/runtime/reactivity.ts b/src/runtime/reactivity.ts
index 9588d6cc8..15ecb1121 100644
--- a/src/runtime/reactivity.ts
+++ b/src/runtime/reactivity.ts
@@ -11,8 +11,8 @@ const NO_CALLBACK = () => {
 
 // The following types only exist to signify places where objects are expected
 // to be reactive or not, they provide no type checking benefit over "object"
-type Target = object;
-type Reactive<T extends Target> = T;
+export type Target = object;
+export type Reactive<T extends Target> = T;
 
 type Collection = Set<any> | Map<any, any> | WeakMap<any, any>;
 type CollectionRawType = "Set" | "Map" | "WeakMap";
@@ -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.
@@ -127,8 +140,7 @@ function notifyReactives(target: Target, key: PropertyKey): void {
   }
   // Loop on copy because clearReactivesForCallback will modify the set in place
   for (const callback of [...callbacks]) {
-    clearReactivesForCallback(callback);
-    callback();
+    clearAndCall(callback);
   }
 }
 
@@ -176,6 +188,7 @@ export function getSubscriptions(callback: Callback) {
 // Maps reactive objects to the underlying target
 export const targets = new WeakMap<Reactive<Target>, Target>();
 const reactiveCache = new WeakMap<Target, WeakMap<Callback, Reactive<Target>>>();
+const proxyToCallback = new WeakMap<Reactive<Target>, Callback>();
 /**
  * Creates a reactive proxy for an object. Reading data on the reactive object
  * subscribes to changes to the data. Writing data on the object will cause the
@@ -225,10 +238,27 @@ export function reactive<T extends Target>(target: T, callback: Callback = NO_CA
       : basicProxyHandler<T>(callback);
     const proxy = new Proxy(target, handler as ProxyHandler<T>) as Reactive<T>;
     reactivesForTarget.set(callback, proxy);
+    proxyToCallback.set(proxy, callback);
     targets.set(proxy, target);
   }
   return reactivesForTarget.get(callback) as Reactive<T>;
 }
+
+/**
+ * Creates a target that will notify multiple reactives when dependencies change.
+ */
+export function multiReactive<T extends Target>(
+  reactiveTarget: T | Reactive<T>,
+  callback: Callback
+): T {
+  const existingCB = proxyToCallback.get(reactiveTarget);
+  if (existingCB && existingCB !== NO_CALLBACK) {
+    return reactive(reactiveTarget, [callback, existingCB]);
+  } else {
+    return reactive(reactiveTarget, callback);
+  }
+}
+
 /**
  * Creates a basic proxy handler for regular objects and arrays.
  *
diff --git a/src/runtime/utils.ts b/src/runtime/utils.ts
index 7aa9b112f..a2d6e0039 100644
--- a/src/runtime/utils.ts
+++ b/src/runtime/utils.ts
@@ -1,5 +1,5 @@
 import { OwlError } from "../common/owl_error";
-export type Callback = () => 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
@@ -8,9 +8,9 @@ export type Callback = () => void;
  * @param callback the callback to batch
  * @returns a batched version of the original callback
  */
-export function batched(callback: Callback): Callback {
+export function batched<Args extends any[]>(callback: (...args: Args) => any) {
   let scheduled = false;
-  return async (...args) => {
+  return async (...args: Args) => {
     if (!scheduled) {
       scheduled = true;
       await Promise.resolve();
diff --git a/tests/computed.test.ts b/tests/computed.test.ts
new file mode 100644
index 000000000..7f7e1ac0a
--- /dev/null
+++ b/tests/computed.test.ts
@@ -0,0 +1,460 @@
+import { reactive, computed, mount, Component, useState, onRendered, xml } from "../src";
+import { batched } from "../src/runtime/utils";
+import { makeTestFixture, nextMicroTick, nextTick } from "./helpers";
+
+function effect<T extends object>(dep: T, fn: (o: T) => void) {
+  const r: any = reactive(dep, () => fn(r));
+  fn(r);
+}
+
+describe("computed - with effect", () => {
+  let orderComputeCounts = { itemTotal: 0, orderTotal: 0 };
+  const resetOrderComputeCounts = () => {
+    orderComputeCounts.itemTotal = 0;
+    orderComputeCounts.orderTotal = 0;
+  };
+
+  const expectOrderComputeCounts = (expected: { itemTotal: number; orderTotal: number }) => {
+    expect(orderComputeCounts).toEqual(expected);
+    resetOrderComputeCounts();
+  };
+
+  beforeEach(() => {
+    resetOrderComputeCounts();
+  });
+
+  type Product = { unitPrice: number };
+  type OrderItem = { product: Product; quantity: number };
+  type Order = { items: OrderItem[]; discount: number };
+
+  const createProduct = (unitPrice: number): Product => {
+    return reactive({ unitPrice });
+  };
+
+  const createOrderItem = (product: Product, quantity: number) => {
+    return reactive({ product, quantity });
+  };
+
+  const createOrder = (): Order => {
+    return reactive({ items: [], discount: 0 });
+  };
+
+  const getItemTotal = computed((item: OrderItem) => {
+    orderComputeCounts.itemTotal++;
+
+    return item.product.unitPrice * item.quantity;
+  });
+
+  const getOrderTotal = computed((order: Order) => {
+    orderComputeCounts.orderTotal++;
+
+    let result = 0;
+    for (let item of order.items) {
+      result += getItemTotal(item);
+    }
+    return result * (1 - order.discount / 100);
+  });
+
+  test("effect depends on getter", () => {
+    let distanceComputeCount = 0;
+    const expectDistanceComputeCount = (expected: number) => {
+      expect(distanceComputeCount).toBe(expected);
+      distanceComputeCount = 0;
+    };
+
+    // point <- computed distance <- computed deepComputedVal
+    const point = reactive({ x: 0, y: 0 });
+    const distance = computed((p: typeof point) => {
+      distanceComputeCount++;
+      return Math.sqrt(Math.pow(p.x, 2) + Math.pow(p.y, 2));
+    });
+    const deepComputedVal = computed((p: typeof point) => {
+      // absurd computation to test that the getter is not recomputed
+      let result = 0;
+      for (let i = 0; i < 5; i++) {
+        result += distance(p);
+      }
+      return result;
+    });
+
+    let val = 0;
+    effect(point, (p) => {
+      // Notice that in this effect, only the `deepComputedVal` is directly used.
+      // It is indirectly dependent on the `x` and `y` of `p`.
+      // Nevertheless, mutating `x` or `y` should trigger this effect.
+      val = deepComputedVal(p);
+    });
+
+    expect(val).toEqual(0);
+    expectDistanceComputeCount(1);
+    expect(distance(point)).toEqual(0);
+    // No recomputation even after the previous `distance` call.
+    expectDistanceComputeCount(0);
+
+    point.x = 3;
+    expect(val).toEqual(15);
+    expectDistanceComputeCount(1);
+    expect(distance(point)).toEqual(3);
+    // No recomputation even after the previous `distance` call.
+    expectDistanceComputeCount(0);
+
+    point.y = 4;
+    expect(val).toEqual(25);
+    expectDistanceComputeCount(1);
+    expect(distance(point)).toEqual(5);
+    // No recomputation even after the previous `distance` call.
+    expectDistanceComputeCount(0);
+  });
+
+  test("can depend on network of objects", () => {
+    const p1 = createProduct(10);
+    const p2 = createProduct(20);
+    const p3 = createProduct(30);
+    const o = createOrder();
+    o.items.push(createOrderItem(p1, 1));
+    o.items.push(createOrderItem(p2, 2));
+    o.items.push(createOrderItem(p3, 3));
+
+    let orderTotal = 0;
+    effect(o, (o) => {
+      orderTotal = getOrderTotal(o);
+    });
+
+    expect(orderTotal).toEqual(140);
+
+    p1.unitPrice = 11;
+    expect(orderTotal).toEqual(141);
+
+    o.items[1].quantity = 4;
+    expect(orderTotal).toEqual(181);
+
+    o.items.push(createOrderItem(createProduct(40), 1));
+    expect(orderTotal).toEqual(221);
+  });
+
+  test("batched effect", async () => {
+    const p1 = createProduct(10);
+    const p2 = createProduct(20);
+    const p3 = createProduct(30);
+    const o = createOrder();
+    o.items.push(createOrderItem(p1, 1));
+    o.items.push(createOrderItem(p2, 2));
+    o.items.push(createOrderItem(p3, 3));
+
+    let orderTotal = 0;
+    effect(
+      o,
+      batched((o) => {
+        orderTotal = getOrderTotal(o);
+      })
+    );
+
+    await nextMicroTick();
+
+    expect(orderTotal).toEqual(140);
+    expectOrderComputeCounts({ itemTotal: 3, orderTotal: 1 });
+
+    p1.unitPrice = 11;
+    await nextMicroTick();
+
+    expect(orderTotal).toEqual(141);
+    expectOrderComputeCounts({ itemTotal: 1, orderTotal: 1 });
+
+    o.items[1].quantity = 4;
+    p3.unitPrice = 31;
+    await nextMicroTick();
+
+    expect(orderTotal).toEqual(184);
+    expectOrderComputeCounts({ itemTotal: 2, orderTotal: 1 });
+
+    o.items.push(createOrderItem(createProduct(40), 1));
+    o.items[0].quantity = 5;
+    p2.unitPrice = 21;
+    await nextMicroTick();
+
+    expect(orderTotal).toEqual(272);
+    expectOrderComputeCounts({ itemTotal: 3, orderTotal: 1 });
+    expect(getOrderTotal(o)).toEqual(272);
+    // No recomputation even after the previous `getOrderTotal` call.
+    expectOrderComputeCounts({ itemTotal: 0, orderTotal: 0 });
+
+    o.discount = 10;
+    await nextMicroTick();
+    expect(orderTotal).toEqual(244.8);
+    expectOrderComputeCounts({ itemTotal: 0, orderTotal: 1 });
+  });
+});
+
+describe("computed - with components", () => {
+  type State = { a: number; b: number };
+
+  let fixture: HTMLElement;
+  let computeCounts = { c: 0, d: 0, e: 0, f: 0 };
+
+  const expectComputeCounts = (expected: { c: number; d: number; e: number; f: number }) => {
+    expect(computeCounts).toEqual(expected);
+    computeCounts = { c: 0, d: 0, e: 0, f: 0 };
+  };
+
+  beforeEach(() => {
+    fixture = makeTestFixture();
+    computeCounts = { c: 0, d: 0, e: 0, f: 0 };
+  });
+
+  const c = computed((self: State) => {
+    computeCounts.c++;
+    return self.a + self.b;
+  });
+  const d = computed((self: State) => {
+    computeCounts.d++;
+    return 2 * self.a;
+  });
+  const e = computed((self: State) => {
+    computeCounts.e++;
+    let result = 0;
+    for (let i = 0; i < 5; i++) {
+      result += c(self) + d(self);
+    }
+    return result;
+  });
+  const f = computed((self: State) => {
+    computeCounts.f++;
+    let result = 0;
+    for (let i = 0; i < 10; i++) {
+      result += d(self);
+    }
+    return result;
+  });
+
+  const updateA = (self: State, by: number) => {
+    self.a += by;
+  };
+  const updateB = (self: State, by: number) => {
+    self.b += by;
+  };
+
+  const createComponents = (state: State) => {
+    let renderCounts = { App: 0, C: 0, D: 0, E: 0, F: 0 };
+    const expectRenderCounts = (expected: {
+      App: number;
+      C: number;
+      D: number;
+      E: number;
+      F: number;
+    }) => {
+      expect(renderCounts).toEqual(expected);
+      renderCounts = { App: 0, C: 0, D: 0, E: 0, F: 0 };
+    };
+
+    class BaseComp extends Component {
+      state = useState(state);
+      setup() {
+        Object.assign(this, { c, d, e, f });
+        onRendered(() => {
+          const name = (this.constructor as any).name as keyof typeof renderCounts;
+          renderCounts[name]++;
+        });
+      }
+    }
+    class C extends BaseComp {
+      static components = {};
+    }
+    class D extends BaseComp {
+      static components = {};
+    }
+    class E extends BaseComp {
+      static components = {};
+    }
+    class F extends BaseComp {
+      static components = {};
+    }
+    class App extends BaseComp {
+      static components = {};
+    }
+    return { App, C, D, E, F, expectRenderCounts };
+  };
+
+  test("number of rerendering - sibling components", async () => {
+    const state = reactive({ a: 1, b: 1 });
+    const { App, C, D, E, F, expectRenderCounts } = createComponents(state);
+
+    C.template = xml`<p t-out="c(state)" />`;
+
+    D.template = xml`<p t-out="d(state)" />`;
+
+    E.template = xml`<p t-out="e(state)" />`;
+
+    F.template = xml`<p t-out="f(state)" />`;
+
+    App.template = xml`<div><C /><D /><E /><F /></div>`;
+    App.components = { C, D, E, F };
+
+    await mount(App, fixture);
+    expectRenderCounts({ App: 1, C: 1, D: 1, E: 1, F: 1 });
+
+    updateA(state, 1);
+    await nextTick();
+    // App doesn't depend on the state, therefore it doesn't re-render
+    expectRenderCounts({ App: 0, C: 1, D: 1, E: 1, F: 1 });
+
+    updateB(state, 1);
+    await nextTick();
+    // changing b doesn't affect d
+    expectRenderCounts({ App: 0, C: 1, D: 0, E: 1, F: 0 });
+
+    // Multiple changes should only render once.
+    for (let i = 0; i < 10; i++) {
+      updateA(state, 1);
+    }
+    await nextTick();
+    expectRenderCounts({ App: 0, C: 1, D: 1, E: 1, F: 1 });
+  });
+
+  test("number of rerenderings - nested components", async () => {
+    const state = reactive({ a: 1, b: 1 });
+    const { App, C, D, E, F, expectRenderCounts } = createComponents(state);
+
+    F.template = xml`<p t-out="f(state)" />`;
+
+    E.template = xml`<div><p t-out="e(state)" /><F /></div>`;
+    E.components = { F };
+
+    D.template = xml`<div><p t-out="d(state)" /><E /></div>`;
+    D.components = { E };
+
+    C.template = xml`<div><p t-out="c(state)" /><D /></div>`;
+    C.components = { D };
+
+    App.template = xml`<div><C /></div>`;
+    App.components = { C };
+
+    await mount(App, fixture);
+    expectRenderCounts({ App: 1, C: 1, D: 1, E: 1, F: 1 });
+
+    updateA(state, 1);
+    await nextTick();
+    // App doesn't depend on the state, therefore it doesn't re-render
+    expectRenderCounts({ App: 0, C: 1, D: 1, E: 1, F: 1 });
+
+    updateB(state, 1);
+    await nextTick();
+    // changing b doesn't affect d
+    expectRenderCounts({ App: 0, C: 1, D: 0, E: 1, F: 0 });
+
+    // Multiple changes should only render once.
+    for (let i = 0; i < 10; i++) {
+      updateA(state, 1);
+    }
+    await nextTick();
+    expectRenderCounts({ App: 0, C: 1, D: 1, E: 1, F: 1 });
+  });
+
+  test("number of compute calls in components", async () => {
+    const state = reactive({ a: 1, b: 1 });
+    const { App, C, D, E, F } = createComponents(state);
+
+    C.template = xml`<p t-out="c(state)" />`;
+
+    D.template = xml`<p t-out="d(state)" />`;
+
+    E.template = xml`<p t-out="e(state)" />`;
+
+    F.template = xml`<p t-out="f(state)" />`;
+
+    App.template = xml`<div><C /><D /><E /><F /></div>`;
+    App.components = { C, D, E, F };
+
+    await mount(App, fixture);
+    expectComputeCounts({ c: 1, d: 1, e: 1, f: 1 });
+
+    updateA(state, 1);
+    await nextTick();
+    // everything will be re-computed
+    expectComputeCounts({ c: 1, d: 1, e: 1, f: 1 });
+
+    updateB(state, 1);
+    await nextTick();
+    // only c and e will be re-computed
+    expectComputeCounts({ c: 1, d: 0, e: 1, f: 0 });
+
+    // update both
+    updateA(state, 1);
+    updateB(state, 1);
+    await nextTick();
+    // 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 });
+  });
+});
diff --git a/tests/reactivity.test.ts b/tests/reactivity.test.ts
index bad028cd8..22d019cc6 100644
--- a/tests/reactivity.test.ts
+++ b/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,
@@ -2424,3 +2424,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 });
+  });
+});

From c38f02165d0b576f44b4e984e5db400916709599 Mon Sep 17 00:00:00 2001
From: Joseph Caburnay <jcb@odoo.com>
Date: Wed, 3 Jan 2024 18:23:36 +0100
Subject: [PATCH 2/3] add new test for computed that sorts an array

I had a discussion with SEB and he told me about auto-sort compute. He said that
it would be better if we could implement automatic in-place sorting without
triggering the reactivity.

However, this PR can't actually handle that because this PR is just an extension
of the existing reactivity, it doesn't actually modify it. So if inplace sort
will result to multiple mutations, then that will be the number of times the
callback will be called. Even inside a `computed` that's introduced in this PR.

Nevertheless, compute that depends on sorted array is still useful, it's just
that the sorting should not be done in-place. This commit introduces an example
where a computed value that returns a sorted array is only called once (in a
batched context) even though multiple mutations are made to the array.
---
 tests/computed.test.ts | 87 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/tests/computed.test.ts b/tests/computed.test.ts
index 7f7e1ac0a..4e8e57ef3 100644
--- a/tests/computed.test.ts
+++ b/tests/computed.test.ts
@@ -183,6 +183,93 @@ describe("computed - with effect", () => {
     expect(orderTotal).toEqual(244.8);
     expectOrderComputeCounts({ itemTotal: 0, orderTotal: 1 });
   });
+
+  test("computed sorted array", async () => {
+    let sortingCount = 0;
+    const expectSortingCount = (expected: number) => {
+      expect(sortingCount).toBe(expected);
+      sortingCount = 0;
+    };
+
+    const array = reactive([
+      52, 26, 71, 63, 72, 57, 71, 11, 17, 30, 52, 90, 14, 33, 86, 13, 62, 34, 99, 61, 21, 92, 95,
+      99, 0, 92, 6, 35, 95, 39, 87, 30, 50, 74, 21, 67, 34, 98, 99, 46, 85, 63, 41, 56, 18, 43, 23,
+      59, 52, 12,
+    ]);
+
+    const sortedArray = computed((a: typeof array) => {
+      sortingCount++;
+      const copy = [...a];
+      return copy.sort((a, b) => a - b);
+    });
+
+    const range = computed((a: typeof array) => {
+      a = sortedArray(a);
+      return a[a.length - 1] - a[0];
+    });
+
+    const average = computed((a: typeof array) => {
+      let sum = 0;
+      for (let i = 0; i < a.length; i++) {
+        sum += a[i];
+      }
+      return Math.trunc(sum / a.length);
+    });
+
+    const min = computed((a: typeof array) => {
+      a = sortedArray(a);
+      return a[0];
+    });
+
+    const max = computed((a: typeof array) => {
+      a = sortedArray(a);
+      return a[a.length - 1];
+    });
+
+    const statTotal = computed((a: typeof array) => {
+      return range(a) + average(a) + min(a) + max(a);
+    });
+
+    let val = 0;
+    effect(
+      array,
+      batched((a) => {
+        val = statTotal(a) + statTotal(a) + statTotal(a);
+      })
+    );
+
+    await nextMicroTick();
+
+    expectSortingCount(1);
+    expect(val).toEqual(250 * 3);
+
+    array.push(99, 100);
+    await nextMicroTick();
+
+    expectSortingCount(1);
+    expect(val).toEqual(254 * 3);
+
+    array.push(-50);
+    await nextMicroTick();
+
+    expectSortingCount(1);
+    expect(val).toEqual(252 * 3);
+
+    // After some mutations, the original order is kept because the `sortedArray` getter makes a copy of the array before sorting.
+    const arrayCopy = [...array];
+    for (let i = 0; i < array.length; i++) {
+      expect(array[i]).toEqual(arrayCopy[i]);
+    }
+
+    // Sort will perform so many mutations in the original array.
+    // This will register several of recomputations.
+    // But since the effect is batched, it will only be recomputed once.
+    array.sort((a, b) => b - a);
+    await nextMicroTick();
+
+    expectSortingCount(1);
+    expect(val).toEqual(252 * 3);
+  });
 });
 
 describe("computed - with components", () => {

From f12497286202f723b1f3cb6881f91347078e25b7 Mon Sep 17 00:00:00 2001
From: Joseph Caburnay <caburnay.joseph@gmail.com>
Date: Fri, 5 Jan 2024 12:18:45 +0100
Subject: [PATCH 3/3] sam's lazyComputed

---
 tests/computed.test.ts | 248 +++++++++++++++++++++++------------------
 1 file changed, 140 insertions(+), 108 deletions(-)

diff --git a/tests/computed.test.ts b/tests/computed.test.ts
index 4e8e57ef3..87945855b 100644
--- a/tests/computed.test.ts
+++ b/tests/computed.test.ts
@@ -1,10 +1,35 @@
-import { reactive, computed, mount, Component, useState, onRendered, xml } from "../src";
+import { Component, mount, onRendered, reactive, useState, xml } from "../src";
 import { batched } from "../src/runtime/utils";
 import { makeTestFixture, nextMicroTick, nextTick } from "./helpers";
 
-function effect<T extends object>(dep: T, fn: (o: T) => void) {
-  const r: any = reactive(dep, () => fn(r));
-  fn(r);
+function effect(cb: any, deps: any) {
+  const reactiveDeps = reactive(deps, function recompute() {
+    cb(...reactiveDeps);
+  });
+  cb(...reactiveDeps);
+}
+
+function lazyComputed(obj: any, propName: string, compute: any) {
+  const key = Symbol(propName);
+  Object.defineProperty(obj, propName, {
+    get() {
+      return this[key]();
+    },
+    configurable: true,
+  });
+
+  effect(
+    function recompute(obj: any) {
+      const value: any[] = [];
+      obj[key] = () => {
+        if (!value.length) {
+          value.push(compute(obj));
+        }
+        return value[0];
+      };
+    },
+    [obj]
+  );
 }
 
 describe("computed - with effect", () => {
@@ -24,36 +49,36 @@ describe("computed - with effect", () => {
   });
 
   type Product = { unitPrice: number };
-  type OrderItem = { product: Product; quantity: number };
-  type Order = { items: OrderItem[]; discount: number };
+  type OrderItem = { product: Product; quantity: number; itemTotal: number };
+  type Order = { items: any[]; discount: number; orderTotal: number };
 
   const createProduct = (unitPrice: number): Product => {
     return reactive({ unitPrice });
   };
 
   const createOrderItem = (product: Product, quantity: number) => {
-    return reactive({ product, quantity });
-  };
+    const item = reactive({ product, quantity });
+    lazyComputed(item, "itemTotal", (item: OrderItem) => {
+      orderComputeCounts.itemTotal++;
 
-  const createOrder = (): Order => {
-    return reactive({ items: [], discount: 0 });
+      return item.product.unitPrice * item.quantity;
+    });
+    return item;
   };
 
-  const getItemTotal = computed((item: OrderItem) => {
-    orderComputeCounts.itemTotal++;
-
-    return item.product.unitPrice * item.quantity;
-  });
-
-  const getOrderTotal = computed((order: Order) => {
-    orderComputeCounts.orderTotal++;
+  const createOrder = () => {
+    const order = reactive({ items: [], discount: 0 });
+    lazyComputed(order, "orderTotal", (order: Order) => {
+      orderComputeCounts.orderTotal++;
 
-    let result = 0;
-    for (let item of order.items) {
-      result += getItemTotal(item);
-    }
-    return result * (1 - order.discount / 100);
-  });
+      let result = 0;
+      for (let item of order.items) {
+        result += item.itemTotal;
+      }
+      return result * (1 - order.discount / 100);
+    });
+    return order as unknown as Order;
+  };
 
   test("effect depends on getter", () => {
     let distanceComputeCount = 0;
@@ -63,45 +88,49 @@ describe("computed - with effect", () => {
     };
 
     // point <- computed distance <- computed deepComputedVal
-    const point = reactive({ x: 0, y: 0 });
-    const distance = computed((p: typeof point) => {
+    const point = reactive({ x: 0, y: 0 }) as any;
+    lazyComputed(point, "distance", (p: typeof point) => {
       distanceComputeCount++;
       return Math.sqrt(Math.pow(p.x, 2) + Math.pow(p.y, 2));
     });
-    const deepComputedVal = computed((p: typeof point) => {
+    lazyComputed(point, "deepComputedVal", (p: typeof point) => {
       // absurd computation to test that the getter is not recomputed
       let result = 0;
       for (let i = 0; i < 5; i++) {
-        result += distance(p);
+        // @ts-ignore
+        result += p.distance;
       }
       return result;
     });
 
     let val = 0;
-    effect(point, (p) => {
-      // Notice that in this effect, only the `deepComputedVal` is directly used.
-      // It is indirectly dependent on the `x` and `y` of `p`.
-      // Nevertheless, mutating `x` or `y` should trigger this effect.
-      val = deepComputedVal(p);
-    });
+    effect(
+      (p: any) => {
+        // Notice that in this effect, only the `deepComputedVal` is directly used.
+        // It is indirectly dependent on the `x` and `y` of `p`.
+        // Nevertheless, mutating `x` or `y` should trigger this effect.
+        val = p.deepComputedVal;
+      },
+      [point]
+    );
 
     expect(val).toEqual(0);
     expectDistanceComputeCount(1);
-    expect(distance(point)).toEqual(0);
+    expect(point.distance).toEqual(0);
     // No recomputation even after the previous `distance` call.
     expectDistanceComputeCount(0);
 
     point.x = 3;
     expect(val).toEqual(15);
     expectDistanceComputeCount(1);
-    expect(distance(point)).toEqual(3);
+    expect(point.distance).toEqual(3);
     // No recomputation even after the previous `distance` call.
     expectDistanceComputeCount(0);
 
     point.y = 4;
     expect(val).toEqual(25);
     expectDistanceComputeCount(1);
-    expect(distance(point)).toEqual(5);
+    expect(point.distance).toEqual(5);
     // No recomputation even after the previous `distance` call.
     expectDistanceComputeCount(0);
   });
@@ -116,9 +145,12 @@ describe("computed - with effect", () => {
     o.items.push(createOrderItem(p3, 3));
 
     let orderTotal = 0;
-    effect(o, (o) => {
-      orderTotal = getOrderTotal(o);
-    });
+    effect(
+      (o: any) => {
+        orderTotal = o.orderTotal;
+      },
+      [o]
+    );
 
     expect(orderTotal).toEqual(140);
 
@@ -143,10 +175,10 @@ describe("computed - with effect", () => {
 
     let orderTotal = 0;
     effect(
-      o,
       batched((o) => {
-        orderTotal = getOrderTotal(o);
-      })
+        orderTotal = o.orderTotal;
+      }),
+      [o]
     );
 
     await nextMicroTick();
@@ -174,7 +206,7 @@ describe("computed - with effect", () => {
 
     expect(orderTotal).toEqual(272);
     expectOrderComputeCounts({ itemTotal: 3, orderTotal: 1 });
-    expect(getOrderTotal(o)).toEqual(272);
+    expect(o.orderTotal).toEqual(272);
     // No recomputation even after the previous `getOrderTotal` call.
     expectOrderComputeCounts({ itemTotal: 0, orderTotal: 0 });
 
@@ -197,18 +229,19 @@ describe("computed - with effect", () => {
       59, 52, 12,
     ]);
 
-    const sortedArray = computed((a: typeof array) => {
+    lazyComputed(array, "sortedArray", (a: typeof array) => {
       sortingCount++;
       const copy = [...a];
       return copy.sort((a, b) => a - b);
     });
 
-    const range = computed((a: typeof array) => {
-      a = sortedArray(a);
+    lazyComputed(array, "range", (a: typeof array) => {
+      // @ts-ignore
+      a = a.sortedArray;
       return a[a.length - 1] - a[0];
     });
 
-    const average = computed((a: typeof array) => {
+    lazyComputed(array, "average", (a: typeof array) => {
       let sum = 0;
       for (let i = 0; i < a.length; i++) {
         sum += a[i];
@@ -216,26 +249,29 @@ describe("computed - with effect", () => {
       return Math.trunc(sum / a.length);
     });
 
-    const min = computed((a: typeof array) => {
-      a = sortedArray(a);
+    lazyComputed(array, "min", (a: typeof array) => {
+      // @ts-ignore
+      a = a.sortedArray;
       return a[0];
     });
 
-    const max = computed((a: typeof array) => {
-      a = sortedArray(a);
+    lazyComputed(array, "max", (a: typeof array) => {
+      // @ts-ignore
+      a = a.sortedArray;
       return a[a.length - 1];
     });
 
-    const statTotal = computed((a: typeof array) => {
-      return range(a) + average(a) + min(a) + max(a);
+    lazyComputed(array, "statTotal", (a: typeof array) => {
+      // @ts-ignore
+      return a.range + a.average + a.min + a.max;
     });
 
     let val = 0;
     effect(
-      array,
       batched((a) => {
-        val = statTotal(a) + statTotal(a) + statTotal(a);
-      })
+        val = a.statTotal + a.statTotal + a.statTotal;
+      }),
+      [array]
     );
 
     await nextMicroTick();
@@ -288,31 +324,6 @@ describe("computed - with components", () => {
     computeCounts = { c: 0, d: 0, e: 0, f: 0 };
   });
 
-  const c = computed((self: State) => {
-    computeCounts.c++;
-    return self.a + self.b;
-  });
-  const d = computed((self: State) => {
-    computeCounts.d++;
-    return 2 * self.a;
-  });
-  const e = computed((self: State) => {
-    computeCounts.e++;
-    let result = 0;
-    for (let i = 0; i < 5; i++) {
-      result += c(self) + d(self);
-    }
-    return result;
-  });
-  const f = computed((self: State) => {
-    computeCounts.f++;
-    let result = 0;
-    for (let i = 0; i < 10; i++) {
-      result += d(self);
-    }
-    return result;
-  });
-
   const updateA = (self: State, by: number) => {
     self.a += by;
   };
@@ -321,6 +332,30 @@ describe("computed - with components", () => {
   };
 
   const createComponents = (state: State) => {
+    lazyComputed(state, "c", (self: any) => {
+      computeCounts.c++;
+      return self.a + self.b;
+    });
+    lazyComputed(state, "d", (self: any) => {
+      computeCounts.d++;
+      return 2 * self.a;
+    });
+    lazyComputed(state, "e", (self: any) => {
+      computeCounts.e++;
+      let result = 0;
+      for (let i = 0; i < 5; i++) {
+        result += self.c + self.d;
+      }
+      return result;
+    });
+    lazyComputed(state, "f", (self: any) => {
+      computeCounts.f++;
+      let result = 0;
+      for (let i = 0; i < 10; i++) {
+        result += self.d;
+      }
+      return result;
+    });
     let renderCounts = { App: 0, C: 0, D: 0, E: 0, F: 0 };
     const expectRenderCounts = (expected: {
       App: number;
@@ -336,7 +371,6 @@ describe("computed - with components", () => {
     class BaseComp extends Component {
       state = useState(state);
       setup() {
-        Object.assign(this, { c, d, e, f });
         onRendered(() => {
           const name = (this.constructor as any).name as keyof typeof renderCounts;
           renderCounts[name]++;
@@ -365,13 +399,13 @@ describe("computed - with components", () => {
     const state = reactive({ a: 1, b: 1 });
     const { App, C, D, E, F, expectRenderCounts } = createComponents(state);
 
-    C.template = xml`<p t-out="c(state)" />`;
+    C.template = xml`<p t-out="state.c" />`;
 
-    D.template = xml`<p t-out="d(state)" />`;
+    D.template = xml`<p t-out="state.d" />`;
 
-    E.template = xml`<p t-out="e(state)" />`;
+    E.template = xml`<p t-out="state.e" />`;
 
-    F.template = xml`<p t-out="f(state)" />`;
+    F.template = xml`<p t-out="state.f" />`;
 
     App.template = xml`<div><C /><D /><E /><F /></div>`;
     App.components = { C, D, E, F };
@@ -401,15 +435,15 @@ describe("computed - with components", () => {
     const state = reactive({ a: 1, b: 1 });
     const { App, C, D, E, F, expectRenderCounts } = createComponents(state);
 
-    F.template = xml`<p t-out="f(state)" />`;
+    F.template = xml`<p t-out="state.f" />`;
 
-    E.template = xml`<div><p t-out="e(state)" /><F /></div>`;
+    E.template = xml`<div><p t-out="state.e" /><F /></div>`;
     E.components = { F };
 
-    D.template = xml`<div><p t-out="d(state)" /><E /></div>`;
+    D.template = xml`<div><p t-out="state.d" /><E /></div>`;
     D.components = { E };
 
-    C.template = xml`<div><p t-out="c(state)" /><D /></div>`;
+    C.template = xml`<div><p t-out="state.c" /><D /></div>`;
     C.components = { D };
 
     App.template = xml`<div><C /></div>`;
@@ -440,13 +474,13 @@ describe("computed - with components", () => {
     const state = reactive({ a: 1, b: 1 });
     const { App, C, D, E, F } = createComponents(state);
 
-    C.template = xml`<p t-out="c(state)" />`;
+    C.template = xml`<p t-out="state.c" />`;
 
-    D.template = xml`<p t-out="d(state)" />`;
+    D.template = xml`<p t-out="state.d" />`;
 
-    E.template = xml`<p t-out="e(state)" />`;
+    E.template = xml`<p t-out="state.e" />`;
 
-    F.template = xml`<p t-out="f(state)" />`;
+    F.template = xml`<p t-out="state.f" />`;
 
     App.template = xml`<div><C /><D /><E /><F /></div>`;
     App.components = { C, D, E, F };
@@ -474,15 +508,16 @@ describe("computed - with components", () => {
 
   test("more complicated compute tree", async () => {
     const state = reactive({ x: 3, y: 2 });
-    const b = computed((self: typeof state) => {
+
+    lazyComputed(state, "b", (self: any) => {
       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");
+    });
+    lazyComputed(state, "e", (self: any) => {
+      return self.b * self.x;
+    });
+    lazyComputed(state, "f", (self: any) => {
+      return self.e + self.b;
+    });
 
     const renderCounts = { A: 0, C: 0 };
     const expectRenderCounts = (expected: { A: number; C: number }) => {
@@ -493,13 +528,10 @@ describe("computed - with components", () => {
 
     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)" />`;
+      static template = xml`<p id="A" t-out="state.f" />`;
       setup() {
         super.setup();
         onRendered(() => {
@@ -509,7 +541,7 @@ describe("computed - with components", () => {
     }
     class C extends BaseComp {
       static components = {};
-      static template = xml`<p id="C" t-out="f(state)" />`;
+      static template = xml`<p id="C" t-out="state.f" />`;
       setup() {
         super.setup();
         onRendered(() => {