Remove subtleWatchDirty again because it is too dangerous.

packages/reactivity-core/CHANGELOG.md

@@ -3,9 +3,8 @@
 ## v0.4.3 (Unreleased)
 
 -   Introduce `synchronized`, a new kind of signal designed to integrate foreign data sources.
--   Introduce `subtleWatchDirty`, a function that allows one to watch for signal changes without triggering the re-evaluation of the signal.
 -   Add missing `forEach` method to `ReactiveSet` and `ReactiveMap`.
--   Deprecate `syncEffectOnce` (use `subtleWatchDirty` instead).
+-   Deprecate `syncEffectOnce` (should not be needed any longer).
 -   Refactor the implementation of `watch`, `watchValue` and `effect` (async variants). This should not have any impact on users of the library.
 
 ## v0.4.2

packages/reactivity-core/ReactiveImpl.test.ts

@@ -455,7 +455,7 @@ describe("synchronized", () => {
         expect(c4.value).toBe(6);
         expect(c4.value).toBe(6);
         expect(source.getterCalled).toMatchInlineSnapshot(`3`);
-        
+
         watchHandle.destroy();
     });
 });

packages/reactivity-core/ReactiveImpl.ts

@@ -1,11 +1,11 @@
 import {
-    ReadonlySignal as RawReadonlySignal,
     Signal as RawSignal,
     batch as rawBatch,
     computed as rawComputed,
     signal as rawSignal,
     untracked as rawUntracked
 } from "@preact/signals-core";
+import { rawComputedWithSubscriptionHook } from "./hacks";
 import {
     AddBrand,
     AddWritableBrand,
@@ -184,32 +184,32 @@ export function external<T>(compute: () => T, options?: ReactiveOptions<T>): Ext
  * - When the signal is not watched in some way, accesses to the `getter` are not cached.
  * - When the signal is being watched (e.g. by an effect), the signal will automatically subscribe to the foreign data source
  *   and cache the current value until it is informed about a change.
- * 
+ *
  * Example:
- * 
+ *
  * ```ts
  * import { synchronized, watchValue } from "@conterra/reactivity-core";
- * 
+ *
  * 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 
+ *         // 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 
+ *             // unsubscribe from changes in the AbortSignal
  *             abortSignal.removeEventListener("abort", callback);
  *         };
  *     }
  * );
- * 
+ *
  * watchValue(
  *     () => aborted.value,
  *     (aborted) => {
@@ -219,11 +219,11 @@ export function external<T>(compute: () => T, options?: ReactiveOptions<T>): Ext
  *         immediate: true
  *     }
  * );
- * 
+ *
  * setTimeout(() => {
  *     abortController.abort();
  * }, 1000);
- * 
+ *
  * // Prints:
  * // Aborted: false
  * // Aborted: true
@@ -452,45 +452,6 @@ class SynchronizedReactiveImpl<T> extends ReactiveImpl<T> {
     };
 }
 
-// Mangled member names. See https://github.com/preactjs/signals/blob/main/mangle.json.
-const _SUBSCRIBE = "S";
-const _UNSUBSCRIBE = "U";
-
-type RawSignalInternals<T> = RawReadonlySignal<T> & {
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    [_SUBSCRIBE](node: any): void;
-
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    [_UNSUBSCRIBE](node: any): void;
-};
-
-// Overrides the subscribe/unsubscribe methods of a signal to allow for custom subscription hooks.
-function rawComputedWithSubscriptionHook<T>(
-    compute: () => T,
-    subscribe: () => () => void
-): RawReadonlySignal<T> {
-    const signal = rawComputed(compute) as RawSignalInternals<T>;
-    const origSubscribe = signal[_SUBSCRIBE];
-    const origUnsubscribe = signal[_UNSUBSCRIBE];
-
-    let subscriptions = 0;
-    let cleanup: (() => void) | undefined;
-    signal[_SUBSCRIBE] = function patchedSubscribe(node: unknown) {
-        origSubscribe.call(this, node);
-        if (subscriptions++ === 0) {
-            cleanup = subscribe();
-        }
-    };
-    signal[_UNSUBSCRIBE] = function patchedUnsubscribe(node: unknown) {
-        origUnsubscribe.call(this, node);
-        if (--subscriptions === 0) {
-            cleanup?.();
-            cleanup = undefined;
-        }
-    };
-    return signal;
-}
-
 function computeWithEquals<T>(compute: () => T, equals: EqualsFunc<T>) {
     let firstExecution = true;
     let currentValue: T;

packages/reactivity-core/async.ts

@@ -13,7 +13,7 @@ import { TaskQueue } from "./utils/TaskQueue";
 import { watchImpl } from "./watch";
 // eslint-disable-next-line @typescript-eslint/no-unused-vars
 import { syncEffect, syncWatch, syncWatchValue } from "./sync";
-import { createWatcher, RawWatcher } from "./watchDirty";
+import { createWatcher, RawWatcher } from "./hacks";
 
 /**
  * Runs the callback function and tracks its reactive dependencies.
@@ -75,7 +75,7 @@ class AsyncEffect {
     private callback: EffectCallback;
 
     /** The cleanup function returned by an earlier effect execution (if any). */
-    private cleanup: CleanupFunc | void | undefined;
+    private cleanup: CleanupFunc | undefined;
 
     /** The watcher that implements notifications when signals change. */
     private watcher: RawWatcher | undefined;
@@ -157,7 +157,10 @@ class AsyncEffect {
     private triggerCallback() {
         if (!this.isDestroyed) {
             this.triggerCleanup();
-            this.cleanup = this.callback();
+            const cleanup = this.callback();
+            if (typeof cleanup === "function") {
+                this.cleanup = cleanup;
+            }
         }
     }
 

packages/reactivity-core/hacks.test.ts

@@ -0,0 +1,175 @@
+import { describe, expect, it, vi } from "vitest";
+import { batch, reactive, synchronized } from "./ReactiveImpl";
+import { createWatcher, rawComputedWithSubscriptionHook } from "./hacks";
+import { syncWatchValue } from "./sync";
+
+describe("Watcher", () => {
+    it("does not trigger the notify callback if nothing happens", () => {
+        const callback = vi.fn();
+        const watcher = createWatcher(callback);
+        watcher.destroy();
+        expect(callback).toHaveBeenCalledTimes(0);
+    });
+
+    it("does not trigger the notify callback if watched signals don't receive updates", () => {
+        const signal = reactive(0);
+        const callback = vi.fn();
+        const watcher = createWatcher(callback);
+
+        const stop = watcher.start();
+        signal.value; // tracked
+        stop();
+
+        watcher.destroy();
+        expect(callback).toHaveBeenCalledTimes(0);
+    });
+
+    it("triggers multiple updates in a batch", () => {
+        const signal = reactive(0);
+        const callback = vi.fn();
+        const watcher = createWatcher(callback);
+
+        const stop = watcher.start();
+        signal.value; // tracked
+        stop();
+
+        batch(() => {
+            signal.value = 1;
+            signal.value = 2;
+        });
+
+        // Would be ideal to only receive a single update, but we cant
+        // force the preact library to do this without outright forking the project.
+        expect(callback).toHaveBeenCalledTimes(2);
+    });
+
+    it("only triggers updates when currently observed signals update", () => {
+        const signal1 = reactive(0);
+        const signal2 = reactive(0);
+        const callback = vi.fn();
+        const watcher = createWatcher(callback);
+
+        // Watch signal1
+        {
+            const stop = watcher.start();
+            signal1.value; // tracked
+            stop();
+        }
+        signal1.value = 1;
+        expect(callback).toHaveBeenCalledTimes(1);
+
+        // Watch signal2
+        {
+            const stop = watcher.start();
+            signal2.value; // tracked
+            stop();
+        }
+        signal1.value = 2; // no effect since signal1 was not used
+        expect(callback).toHaveBeenCalledTimes(1);
+        signal2.value = 2;
+        expect(callback).toHaveBeenCalledTimes(2);
+    });
+
+    it("subscribes on watch and unsubscribes on destroy", () => {
+        let sub = 0;
+        let unsub = 0;
+        const syncSignal = synchronized(
+            () => 0,
+            () => {
+                sub++;
+                return () => unsub++;
+            }
+        );
+        const normalSignal = reactive(0);
+
+        const callback = vi.fn();
+        const watcher = createWatcher(callback);
+
+        {
+            const stop = watcher.start();
+            syncSignal.value;
+            normalSignal.value;
+            stop();
+        }
+        expect(sub).toBe(1);
+        expect(unsub).toBe(0);
+
+        // Do some changes to trigger the patched notify code (which uses the disposed flags ...).
+        normalSignal.value = 1;
+        normalSignal.value = 2;
+        expect(callback).toHaveBeenCalledTimes(2);
+
+        watcher.destroy();
+        expect(sub).toBe(1);
+        expect(unsub).toBe(1);
+    });
+
+    it("subscribes on watch and unsubscribes on change", () => {
+        let sub = 0;
+        let unsub = 0;
+        const syncSignal = synchronized(
+            () => 0,
+            () => {
+                sub++;
+                return () => unsub++;
+            }
+        );
+        const normalSignal = reactive(0);
+
+        const callback = vi.fn();
+        const watcher = createWatcher(callback);
+
+        {
+            const stop = watcher.start();
+            syncSignal.value;
+            normalSignal.value;
+            stop();
+        }
+        expect(sub).toBe(1);
+        expect(unsub).toBe(0);
+
+        // Do some changes to trigger the patched notify code (which uses the disposed flags ...).
+        normalSignal.value = 1;
+        normalSignal.value = 2;
+        expect(callback).toHaveBeenCalledTimes(2);
+
+        {
+            const stop = watcher.start();
+            normalSignal.value;
+            stop();
+        }
+        expect(sub).toBe(1);
+        expect(unsub).toBe(1);
+    });
+});
+
+describe("rawComputedWithSubscriptionHook", () => {
+    it("detects the first subscriber", () => {
+        const onUnsubscribe = vi.fn();
+        const onSubscribe = vi.fn().mockReturnValue(onUnsubscribe);
+        const signal = rawComputedWithSubscriptionHook(() => 0, onSubscribe);
+        expect(onSubscribe).toHaveBeenCalledTimes(0);
+        expect(onUnsubscribe).toHaveBeenCalledTimes(0);
+
+        const { destroy: destroy1 } = syncWatchValue(
+            () => signal.value,
+            () => {}
+        );
+        expect(onSubscribe).toHaveBeenCalledTimes(1);
+
+        // no effect since this is the second subscriber
+        const { destroy: destroy2 } = syncWatchValue(
+            () => signal.value,
+            () => {}
+        );
+        expect(onSubscribe).toHaveBeenCalledTimes(1);
+
+        // no effect since the other subscribe is still running
+        destroy1();
+        expect(onUnsubscribe).toHaveBeenCalledTimes(0);
+
+        // now the last subscriber is gone
+        destroy2();
+        expect(onUnsubscribe).toHaveBeenCalledTimes(1);
+    });
+});