From 6aa7d0725fe57584ebe5171641ddb2984939e7b5 Mon Sep 17 00:00:00 2001 From: Michael Beckemeyer Date: Wed, 9 Oct 2024 12:36:44 +0200 Subject: [PATCH] Remove `subtleWatchDirty` again because it is too dangerous. --- packages/reactivity-core/CHANGELOG.md | 3 +- packages/reactivity-core/ReactiveImpl.test.ts | 2 +- packages/reactivity-core/ReactiveImpl.ts | 63 ++----- packages/reactivity-core/async.ts | 9 +- packages/reactivity-core/hacks.test.ts | 175 ++++++++++++++++++ packages/reactivity-core/hacks.ts | 88 +++++++++ packages/reactivity-core/index.ts | 1 - packages/reactivity-core/watchDirty.test.ts | 79 -------- packages/reactivity-core/watchDirty.ts | 78 -------- playground/integration.ts | 37 +--- 10 files changed, 291 insertions(+), 244 deletions(-) create mode 100644 packages/reactivity-core/hacks.test.ts create mode 100644 packages/reactivity-core/hacks.ts delete mode 100644 packages/reactivity-core/watchDirty.test.ts delete mode 100644 packages/reactivity-core/watchDirty.ts diff --git a/packages/reactivity-core/CHANGELOG.md b/packages/reactivity-core/CHANGELOG.md index 19d637a..7180392 100644 --- a/packages/reactivity-core/CHANGELOG.md +++ b/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 diff --git a/packages/reactivity-core/ReactiveImpl.test.ts b/packages/reactivity-core/ReactiveImpl.test.ts index d287144..16ce46a 100644 --- a/packages/reactivity-core/ReactiveImpl.test.ts +++ b/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(); }); }); diff --git a/packages/reactivity-core/ReactiveImpl.ts b/packages/reactivity-core/ReactiveImpl.ts index 6ecc92b..8c4ad93 100644 --- a/packages/reactivity-core/ReactiveImpl.ts +++ b/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(compute: () => T, options?: ReactiveOptions): 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(compute: () => T, options?: ReactiveOptions): Ext * immediate: true * } * ); - * + * * setTimeout(() => { * abortController.abort(); * }, 1000); - * + * * // Prints: * // Aborted: false * // Aborted: true @@ -452,45 +452,6 @@ class SynchronizedReactiveImpl extends ReactiveImpl { }; } -// Mangled member names. See https://github.com/preactjs/signals/blob/main/mangle.json. -const _SUBSCRIBE = "S"; -const _UNSUBSCRIBE = "U"; - -type RawSignalInternals = RawReadonlySignal & { - // 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( - compute: () => T, - subscribe: () => () => void -): RawReadonlySignal { - const signal = rawComputed(compute) as RawSignalInternals; - 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(compute: () => T, equals: EqualsFunc) { let firstExecution = true; let currentValue: T; diff --git a/packages/reactivity-core/async.ts b/packages/reactivity-core/async.ts index 5a08a80..9c2b500 100644 --- a/packages/reactivity-core/async.ts +++ b/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; + } } } diff --git a/packages/reactivity-core/hacks.test.ts b/packages/reactivity-core/hacks.test.ts new file mode 100644 index 0000000..18626b3 --- /dev/null +++ b/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); + }); +}); diff --git a/packages/reactivity-core/hacks.ts b/packages/reactivity-core/hacks.ts new file mode 100644 index 0000000..b21856d --- /dev/null +++ b/packages/reactivity-core/hacks.ts @@ -0,0 +1,88 @@ +import { + ReadonlySignal as RawReadonlySignal, + computed as rawComputed, + effect as rawEffect +} from "@preact/signals-core"; +import { CleanupHandle } from "./types"; + +/** @internal */ +export interface RawWatcher extends CleanupHandle { + /** + * Starts a tracking context. Use the returned callback to end the context. + * When a signal used in the tracking context changes, the `onNotify` callback is called. + */ + start(): () => void; +} + +/** + * WARNING: The `onNotify` callback is very aggressive: it gets called _within_ a batch(). + * + * @internal + */ +export function createWatcher(onNotify: () => void): RawWatcher { + // Uses the effect's internals to track signal invalidations. + // The effect body is only called once! + // See https://github.com/preactjs/signals/issues/593#issuecomment-2349672856 + let start!: () => () => void; + const destroy = rawEffect(function (this: RawEffectInternals) { + this[_NOTIFY] = onNotify.bind(undefined); // hide 'this' + start = this[_START].bind(this); + }); + return { + destroy, + start + }; +} + +// Mangled member names. See https://github.com/preactjs/signals/blob/main/mangle.json. +const _NOTIFY = "N"; +const _START = "S"; + +interface RawEffectInternals { + // Notifies the effect that a dependency has changed. + // This usually schedules the effect to run again (when not overridden). + [_NOTIFY](): void; + + // Starts the effect and returns a function to stop it again. + // Signal accesses are tracked while the effect is running. + [_START](): () => void; +} + +// Mangled member names. See https://github.com/preactjs/signals/blob/main/mangle.json. +const _SUBSCRIBE = "S"; +const _UNSUBSCRIBE = "U"; + +type RawSignalInternals = RawReadonlySignal & { + // 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. +export function rawComputedWithSubscriptionHook( + compute: () => T, + subscribe: () => () => void +): RawReadonlySignal { + const signal = rawComputed(compute) as RawSignalInternals; + 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; +} diff --git a/packages/reactivity-core/index.ts b/packages/reactivity-core/index.ts index 97f3b7a..b57fd8c 100644 --- a/packages/reactivity-core/index.ts +++ b/packages/reactivity-core/index.ts @@ -48,6 +48,5 @@ export { } from "./ReactiveImpl"; export { syncEffect, syncEffectOnce, syncWatch, syncWatchValue } from "./sync"; export { effect, watch, watchValue, nextTick } from "./async"; -export { subtleWatchDirty } from "./watchDirty"; export * from "./collections"; export * from "./struct"; diff --git a/packages/reactivity-core/watchDirty.test.ts b/packages/reactivity-core/watchDirty.test.ts deleted file mode 100644 index 66296b4..0000000 --- a/packages/reactivity-core/watchDirty.test.ts +++ /dev/null @@ -1,79 +0,0 @@ -import { expect, it, vi } from "vitest"; -import { computed, reactive } from "./ReactiveImpl"; -import { subtleWatchDirty } from "./watchDirty"; - -it("notifies the callback when the signal changed", () => { - const v = reactive(0); - const callback = vi.fn(); - subtleWatchDirty(v, callback); - - expect(callback).toHaveBeenCalledTimes(0); - - v.value = 1; - expect(callback).toHaveBeenCalledTimes(1); - - v.value = 2; - expect(callback).toHaveBeenCalledTimes(2); -}); - -it("stops notifications when the watch is destroyed", () => { - const v = reactive(0); - const callback = vi.fn(); - const { destroy } = subtleWatchDirty(v, callback); - - v.value = 1; - expect(callback).toHaveBeenCalledTimes(1); - - destroy(); - v.value = 2; - expect(callback).toHaveBeenCalledTimes(1); -}); - -it("does not trigger computed signals", () => { - const count = reactive(0); - const compute = vi.fn(() => count.value + 1); - const signal = computed(compute); - expect(compute).toHaveBeenCalledTimes(0); - - const callback = vi.fn(); - subtleWatchDirty(signal, callback); - expect(callback).toHaveBeenCalledTimes(0); - expect(compute).toHaveBeenCalledTimes(1); // watchDirty accesses the value once during setup - - count.value = 1; - expect(callback).toHaveBeenCalledTimes(1); - expect(compute).toHaveBeenCalledTimes(1); // not called again - - signal.value; - expect(compute).toHaveBeenCalledTimes(2); -}); - -it("tracks the signal even if the initial access throws", () => { - const count = reactive(0); - const compute = vi.fn(() => { - if (count.value === 0) { - throw new Error("oops!"); - } - return count.value + 1; - }); - const signal = computed(compute); - - const callback = vi.fn(); - subtleWatchDirty(signal, callback); - expect(callback).toHaveBeenCalledTimes(0); - - // Was called during setup and the error was ignored. - expect(compute).toHaveBeenCalledTimes(1); - expect(compute.mock.results).toMatchInlineSnapshot(` - [ - { - "type": "throw", - "value": [Error: oops!], - }, - ] - `); - - // Update triggers notification - count.value = 1; - expect(callback).toHaveBeenCalledTimes(1); -}); diff --git a/packages/reactivity-core/watchDirty.ts b/packages/reactivity-core/watchDirty.ts deleted file mode 100644 index d487cdb..0000000 --- a/packages/reactivity-core/watchDirty.ts +++ /dev/null @@ -1,78 +0,0 @@ -import { effect as rawEffect } from "@preact/signals-core"; -import { CleanupHandle, ReadonlyReactive } from "./types"; - -/** - * **Experimental**. - * Notifies the given `callback` whenever the `signal` might have changed, - * without recomputing the signal's current value. - * - * This is a difficult to use, low level API that can be used to build higher level abstractions. - * - * Things to keep in mind when using this function: - * The `callback` should be cheap to invoke (as a signal might change often) and it **must not** throw an exception. - * It should also not make use of any reactive values. - * - * @param signal The signal being watched. - * @param callback The callback to be called whenever the signal might have changed. - * - * @group Watching - */ -export function subtleWatchDirty( - signal: ReadonlyReactive, - callback: () => void -): CleanupHandle { - const watcher = createWatcher(callback); - - const end = watcher.start(); - try { - signal.value; // Tracked - } catch (ignored) { - // We only care about the dependency being set up correctly. - void ignored; - } finally { - end(); - } - - return { - destroy: watcher.destroy - }; -} - -/** @internal */ -export interface RawWatcher extends CleanupHandle { - /** - * Starts a tracking context. Use the returned callback to end the context. - * When a signal used in the tracking context changes, the `onNotify` callback is called. - */ - start(): () => void; -} - -/** @internal */ -export function createWatcher(onNotify: () => void): RawWatcher { - // Uses the effect's internals to track signal invalidations. - // The effect body is only called once! - // See https://github.com/preactjs/signals/issues/593#issuecomment-2349672856 - let start!: () => () => void; - const destroy = rawEffect(function (this: RawEffectInternals) { - this[_NOTIFY] = onNotify.bind(undefined); // hide 'this' - start = this[_START].bind(this); - }); - return { - destroy, - start - }; -} - -// Mangled member names. See https://github.com/preactjs/signals/blob/main/mangle.json. -const _NOTIFY = "N"; -const _START = "S"; - -interface RawEffectInternals { - // Notifies the effect that a dependency has changed. - // This usually schedules the effect to run again (when not overridden). - [_NOTIFY](): void; - - // Starts the effect and returns a function to stop it again. - // Signal accesses are tracked while the effect is running. - [_START](): () => void; -} diff --git a/playground/integration.ts b/playground/integration.ts index f1a6450..f349bb6 100644 --- a/playground/integration.ts +++ b/playground/integration.ts @@ -1,9 +1,5 @@ -import { - ReadonlyReactive, - computed as reactivityComputed, - subtleWatchDirty -} from "@conterra/reactivity-core"; -import { Ref, customRef, onScopeDispose, watchEffect } from "vue"; +import { effect as reactivityEffect } from "@conterra/reactivity-core"; +import { Ref, ShallowRef, onScopeDispose, shallowRef, watchEffect } from "vue"; /** * This composable integrates reactive values into the vue reactivity system. @@ -21,31 +17,14 @@ export function useReactiveSnapshot(compute: () => T): Readonly> { * 1. React to changes of reactive values (from reactivity-core) inside `compute` * 2. React to changes of *vue* values inside `compute` */ - let signal!: ReadonlyReactive; - let trigger!: () => void; - const ref = customRef((track, trigger_) => { - trigger = trigger_; - return { - get() { - track(); - return signal.value; - }, - set() { - throw new Error("Cannot write to the reactive snapshot."); - } - }; - }); + const snapshot = shallowRef() as ShallowRef; const dispose = watchEffect((onCleanup) => { - // Setup the computed signal every time the vue dependencies change. - signal = reactivityComputed(compute); - signal.value; - - // Watch for changes in the reactive signal (non-vue dependencies). - const handle = subtleWatchDirty(signal, trigger); + const handle = reactivityEffect(() => { + const value = compute(); + snapshot.value = value; + }); onCleanup(() => handle.destroy()); - - trigger(); }); onScopeDispose(dispose); - return ref; + return snapshot; }