diff --git a/src/runtime/reactivity.ts b/src/runtime/reactivity.ts index 5e5ad200b..b9fd58a67 100644 --- a/src/runtime/reactivity.ts +++ b/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(); 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 { const existingCB = proxyToCallback.get(reactiveTarget); if (existingCB && existingCB !== NO_CALLBACK) { - const multiCB = new Set(); - 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); } diff --git a/src/runtime/utils.ts b/src/runtime/utils.ts index cf943d2d5..a2d6e0039 100644 --- a/src/runtime/utils.ts +++ b/src/runtime/utils.ts @@ -1,7 +1,5 @@ import { OwlError } from "../common/owl_error"; -export type Callback = SimpleCB | MultiCB; -export type MultiCB = Set; -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 diff --git a/tests/computed.test.ts b/tests/computed.test.ts index b1120144a..7f7e1ac0a 100644 --- a/tests/computed.test.ts +++ b/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`

`; + setup() { + super.setup(); + onRendered(() => { + renderCounts.A++; + }); + } + } + class C extends BaseComp { + static components = {}; + static template = xml`

`; + setup() { + super.setup(); + onRendered(() => { + renderCounts.C++; + }); + } + } + class App extends Component { + static components = { A, C }; + static template = xml`

`; + } + + await mount(App, fixture); + expect(fixture.innerHTML).toEqual('

20

20

'); + expectRenderCounts({ A: 1, C: 1 }); + + state.x = 4; + await nextTick(); + expect(fixture.innerHTML).toEqual('

30

30

'); + 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('

330

330

'); + expectRenderCounts({ A: 1, C: 1 }); + + // Setting without change of value should not re-render. + state.x = 10; + await nextTick(); + expect(fixture.innerHTML).toEqual('

330

330

'); + expectRenderCounts({ A: 0, C: 0 }); + }); }); diff --git a/tests/reactivity.test.ts b/tests/reactivity.test.ts index 9d67f1d52..25dcf6575 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, @@ -2367,3 +2367,104 @@ describe("Reactivity: useState", () => { expect(fixture.innerHTML).toBe("

2b

"); }); }); + +describe("multiReactive", () => { + /** + * A version of effect that combines with the reactive + * object so that both have the same dependencies. + */ + function effectMulti(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 }); + }); +});