From 193a80597982675db7ef512800767481dd0e24f3 Mon Sep 17 00:00:00 2001
From: Andre Frank <a.frank@conterra.de>
Date: Mon, 7 Oct 2024 16:36:31 +0200
Subject: [PATCH] synchronized signal: improve API docs and add unit test

---
 packages/reactivity-core/ReactiveImpl.test.ts | 61 ++++++++++++++++---
 packages/reactivity-core/ReactiveImpl.ts      | 10 +++
 2 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/packages/reactivity-core/ReactiveImpl.test.ts b/packages/reactivity-core/ReactiveImpl.test.ts
index a2d0308..d287144 100644
--- a/packages/reactivity-core/ReactiveImpl.test.ts
+++ b/packages/reactivity-core/ReactiveImpl.test.ts
@@ -1,6 +1,6 @@
 import { describe, expect, it, vi } from "vitest";
 import { batch, computed, external, reactive, synchronized } from "./ReactiveImpl";
-import { syncEffect } from "./sync";
+import { syncEffect, syncWatchValue } from "./sync";
 
 describe("reactive", () => {
     it("supports setting an initial value", () => {
@@ -396,34 +396,74 @@ describe("synchronized", () => {
     });
 
     it("does not cache computes across many levels", () => {
-        const getter = vi.fn().mockReturnValue(1);
-        const sync = synchronized(getter, () => {
-            throw new Error("not called");
-        });
+        const source = new DataSource(1);
+        const sync = synchronized(
+            () => source.value,
+            () => {
+                throw new Error("not called");
+            }
+        );
 
         const c1 = computed(() => sync.value);
         const c2 = computed(() => c1.value);
         const c3 = computed(() => c2.value);
         const c4 = computed(() => c3.value + c2.value);
         expect(c4.value).toBe(2);
-        expect(getter.mock.calls.length).toMatchInlineSnapshot(`2`);
+        expect(source.getterCalled).toMatchInlineSnapshot(`2`);
 
         // High number of re-computations probably due to re-validation of
         // dependencies (see previous test).
         // As long as the value is correct, this is not a major problem.
-        getter.mockReturnValue(2);
+        source.value = 2;
+        expect(c4.value).toBe(4);
+        expect(source.getterCalled).toMatchInlineSnapshot(`8`);
+
+        source.value = 3;
+        expect(c4.value).toBe(6);
+        expect(source.getterCalled).toMatchInlineSnapshot(`14`);
+    });
+
+    it("does cache computes across many levels if the synchronized signal is watched", () => {
+        const source = new DataSource(1);
+        const sync = synchronized(
+            () => source.value,
+            (callback) => {
+                const handle = source.subscribe(callback);
+                return () => handle();
+            }
+        );
+
+        const c1 = computed(() => sync.value);
+        const c2 = computed(() => c1.value);
+        const c3 = computed(() => c2.value);
+        const c4 = computed(() => c3.value + c2.value);
+
+        const watchHandle = syncWatchValue(
+            () => c4.value,
+            () => {}
+        );
+        expect(c4.value).toBe(2);
+        expect(c4.value).toBe(2);
+        expect(source.getterCalled).toMatchInlineSnapshot(`1`);
+
+        source.value = 2;
+        expect(c4.value).toBe(4);
         expect(c4.value).toBe(4);
-        expect(getter.mock.calls.length).toMatchInlineSnapshot(`8`);
+        expect(source.getterCalled).toMatchInlineSnapshot(`2`);
 
-        getter.mockReturnValue(3);
+        source.value = 3;
+        expect(c4.value).toBe(6);
         expect(c4.value).toBe(6);
-        expect(getter.mock.calls.length).toMatchInlineSnapshot(`14`);
+        expect(source.getterCalled).toMatchInlineSnapshot(`3`);
+        
+        watchHandle.destroy();
     });
 });
 
 class DataSource {
     #listener: (() => void) | undefined;
     #value: number;
+    getterCalled = 0;
 
     constructor(value = 0) {
         this.#value = value;
@@ -434,6 +474,7 @@ class DataSource {
     }
 
     get value() {
+        this.getterCalled++;
         return this.#value;
     }
 
diff --git a/packages/reactivity-core/ReactiveImpl.ts b/packages/reactivity-core/ReactiveImpl.ts
index ef2941b..6ecc92b 100644
--- a/packages/reactivity-core/ReactiveImpl.ts
+++ b/packages/reactivity-core/ReactiveImpl.ts
@@ -171,6 +171,8 @@ export function external<T>(compute: () => T, options?: ReactiveOptions<T>): Ext
  * This kind of signal is useful when integrating another library (DOM APIs, etc.) that does not
  * use the reactivity system provided by this library.
  * The major advantage of this API is that it will automatically subscribe to the foreign data source while the signal is actually being used.
+ * It will also automatically unsubscribe when the signal is no longer used.
+ * A signal is considered "used" when it is used by some kind of active effect or watch.
  *
  * Principles:
  * - The `getter` function should return the current value from the foreign data source.
@@ -191,10 +193,18 @@ export function external<T>(compute: () => T, options?: ReactiveOptions<T>): Ext
  * const abortController = new AbortController();
  * const abortSignal = abortController.signal;
  * const aborted = synchronized(
+ * 
+ *     // getter which returns the current value from the foreign source
  *     () => abortSignal.aborted,
+ * 
+ *     // Subscribe function: Automatically called when the signal is used
  *     (callback) => {
+ *         // Subscribe to changes in the AbortSignal 
  *         abortSignal.addEventListener("abort", callback);
+ * 
+ *         // Cleanup function is called automatically when the signal is no longer used
  *         return () => {
+ *             // unsubscribe from changes in the AbortSignal 
  *             abortSignal.removeEventListener("abort", callback);
  *         };
  *     }