diff --git a/.changeset/heavy-turkeys-relate.md b/.changeset/heavy-turkeys-relate.md new file mode 100644 index 000000000..163706f3a --- /dev/null +++ b/.changeset/heavy-turkeys-relate.md @@ -0,0 +1,5 @@ +--- +"@preact/signals-core": minor +--- + +Add support for auto-disposing nested effects diff --git a/.changeset/shiny-jars-invite.md b/.changeset/shiny-jars-invite.md new file mode 100644 index 000000000..6a514e47d --- /dev/null +++ b/.changeset/shiny-jars-invite.md @@ -0,0 +1,7 @@ +--- +"@preact/signals-core": patch +"@preact/signals": patch +"@preact/signals-react": patch +--- + +Remove all usages of `Set`, `Map` and other allocation heavy objects in signals-core. This substaintially increases performance across all measurements. diff --git a/mangle.json b/mangle.json index 3a9e5df39..c48393bde 100644 --- a/mangle.json +++ b/mangle.json @@ -24,17 +24,36 @@ "props": { "cname": 6, "props": { - "$_value": "_v", - "$_deps": "_d", - "$_subs": "_s", - "$_pending": "_p", - "$_updater": "_u", - "$_setCurrent": "_", - "$_activate": "_a", - "$_isComputing": "_c", - "$_readonly": "_r", - "$_requiresUpdate": "_q", - "$_props": "__" + "core: Node": "", + "$_source": "s", + "$_prevSource": "p", + "$_nextSource": "n", + "$_target": "t", + "$_prevTarget": "e", + "$_nextTarget": "x", + "$_rollbackNode": "r", + "core: Signal": "", + "$_value": "v", + "$_node": "n", + "$_targets": "t", + "core: Computed": "", + "$_compute": "v", + "$_globalVersion": "g", + "core: Effect": "", + "$_callback": "c", + "$_nextEffect": "e", + "$_start": "S", + "$_dispose": "d", + "core: Computed+Effect": "", + "$_sources": "s", + "$_notify": "N", + "core: Signal+Node+Computed+Effect": "", + "$_flags": "f", + "$_version": "i", + "$_subscribe": "S", + "$_unsubscribe": "U", + "preact": "", + "$_updaters": "U" } } } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 9d41b915c..6d29e81fb 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -1,295 +1,576 @@ -/** This tracks subscriptions of signals read inside a computed */ -let currentSignal: Signal | undefined; -let commitError: Error | null = null; +function cycleDetected(): never { + throw new Error("Cycle detected"); +} -let batchPending: Set | null = null; +const RUNNING = 1 << 0; +const STALE = 1 << 1; +const NOTIFIED = 1 << 2; +const HAS_ERROR = 1 << 3; +const SHOULD_SUBSCRIBE = 1 << 4; +const SUBSCRIBED = 1 << 5; +const DISPOSED = 1 << 6; + +// A linked list node used to track dependencies (sources) and dependents (targets). +// Also used to remember the source's last version number that the target saw. +type Node = { + // A node may have the following flags: + // SUBSCRIBED when the target has subscribed to listen change notifications from the source + // STALE when it's unclear whether the source is still a dependency of the target + _flags: number; + + // A source whose value the target depends on. + _source: Signal; + _prevSource?: Node; + _nextSource?: Node; + + // A target that depends on the source and should be notified when the source changes. + _target: Computed | Effect; + _prevTarget?: Node; + _nextTarget?: Node; + + // The version number of the source that target has last seen. We use version numbers + // instead of storing the source value, because source values can take arbitrary amount + // of memory, and computeds could hang on to them forever because they're lazily evaluated. + _version: number; + + // Used to remember & roll back the source's previous `._node` value when entering & + // exiting a new evaluation context. + _rollbackNode?: Node; +}; -let oldDeps = new Set(); +function startBatch() { + batchDepth++; +} -export class Signal { - // These property names get minified - see /mangle.json - - /** @internal Internal, do not use. */ - _subs = new Set(); - /** @internal Internal, do not use. */ - _deps = new Set(); - /** @internal Internal, do not use. */ - _pending = 0; - /** @internal Internal, do not use. */ - _value: T; - /** @internal Determine if a computed is allowed to write or not */ - _readonly = false; - /** @internal Marks the signal as requiring an update */ - _requiresUpdate = false; - /** @internal Determine if reads should eagerly activate value */ - _active = false; - /** @internal Used to detect if there is a cycle in the graph */ - _isComputing = false; - - constructor(value: T) { - this._value = value; +function endBatch() { + if (batchDepth > 1) { + batchDepth--; + return; } - toString() { - return "" + this.value; - } + let error: unknown; + let hasError = false; - peek() { - if (!this._active || this._pending > 0) { - activate(this); - } - return this._value; - } + while (batchedEffect !== undefined) { + let effect: Effect | undefined = batchedEffect; + batchedEffect = undefined; - get value() { - if (!this._active || this._pending > 0) { - activate(this); - } + batchIteration++; + + while (effect !== undefined) { + const next: Effect | undefined = effect._nextBatchedEffect; + effect._nextBatchedEffect = undefined; + effect._flags &= ~NOTIFIED; - // If we read a signal outside of a computed we have no way - // to unsubscribe from that. So we assume that the user wants - // to get the value immediately like for testing. - if (!currentSignal) { - return this._value; + if (!(effect._flags & DISPOSED)) { + try { + effect._callback(); + } catch (err) { + if (!hasError) { + error = err; + hasError = true; + } + } + } + effect = next; } + } + batchIteration = 0; + batchDepth--; - // subscribe the current computed to this signal: - this._subs.add(currentSignal); - // update the current computed's dependencies: - currentSignal._deps.add(this); - oldDeps.delete(this); + if (hasError) { + throw error; + } +} - return this._value; +export function batch(callback: () => T): T { + if (batchDepth > 0) { + return callback(); } + /*@__INLINE__**/ startBatch(); + try { + return callback(); + } finally { + endBatch(); + } +} + +// Currently evaluated computed or effect. +let evalContext: Computed | Effect | undefined = undefined; + +// Effects collected into a batch. +let batchedEffect: Effect | undefined = undefined; +let batchDepth = 0; +let batchIteration = 0; + +// A global version number for signals, used for fast-pathing repeated +// computed.peek()/computed.value calls when nothing has changed globally. +let globalVersion = 0; + +function getValue(signal: Signal): T { + let node: Node | undefined = undefined; + if (evalContext !== undefined) { + node = signal._node; + if (node === undefined || node._target !== evalContext) { + // `signal` is a new dependency. Create a new node dependency node, move it + // to the front of the current context's dependency list. + node = { + _flags: 0, + _version: 0, + _source: signal, + _nextSource: evalContext._sources, + _target: evalContext, + _rollbackNode: node, + }; + evalContext._sources = node; + signal._node = node; + + // Subscribe to change notifications from this dependency if we're in an effect + // OR evaluating a computed signal that in turn has subscribers. + if (evalContext._flags & SHOULD_SUBSCRIBE) { + signal._subscribe(node); + } + } else if (node._flags & STALE) { + // `signal` is an existing dependency from a previous evaluation. Reuse the dependency + // node and move it to the front of the evaluation context's dependency list. + node._flags &= ~STALE; + + const head = evalContext._sources; + if (node !== head) { + const prev = node._prevSource; + const next = node._nextSource; + if (prev !== undefined) { + prev._nextSource = next; + } + if (next !== undefined) { + next._prevSource = prev; + } + if (head !== undefined) { + head._prevSource = node; + } + node._prevSource = undefined; + node._nextSource = head; + evalContext._sources = node; + } - set value(value) { - if (this._readonly) { - throw Error("Computed signals are readonly"); + // We can assume that the currently evaluated effect / computed signal is already + // subscribed to change notifications from `signal` if needed. + } else { + // `signal` is an existing dependency from current evaluation. + node = undefined; } + } + try { + return signal.peek(); + } finally { + if (node !== undefined) { + node._version = signal._version; + } + } +} - if (this._value !== value) { - this._value = value; +export class Signal { + /** @internal */ + _value: unknown; - batch(() => { - batchPending!.add(this); + /** @internal */ + _version = 0; - // in batch mode this signal may be marked already - if (this._pending === 0) { - mark(this); - } - }); - } + /** @internal */ + _node?: Node = undefined; + + /** @internal */ + _targets?: Node = undefined; + + constructor(value?: T) { + this._value = value; } - /** - * Start a read operation where this signal is the "current signal" context. - * Returns a function that must be called to end the read context. - * @internal - */ - _setCurrent() { - let prevSignal = currentSignal; - let prevOldDeps = oldDeps; - currentSignal = this; - oldDeps = this._deps; - this._deps = new Set(); - - return (shouldUnmark: boolean, shouldCleanup: boolean) => { - if (shouldUnmark) this._subs.forEach(unmark); - - // Any leftover dependencies here are not needed anymore - if (shouldCleanup) { - // Unsubscribe from dependencies that were not accessed: - oldDeps.forEach(dep => unsubscribe(this, dep)); - } else { - // Re-subscribe to dependencies that were not accessed: - oldDeps.forEach(dep => subscribe(this, dep)); + /** @internal */ + _subscribe(node: Node): void { + if (!(node._flags & SUBSCRIBED)) { + node._flags |= SUBSCRIBED; + node._nextTarget = this._targets; + + if (this._targets !== undefined) { + this._targets._prevTarget = node; } + this._targets = node; + } + } + + /** @internal */ + _unsubscribe(node: Node): void { + if (node._flags & SUBSCRIBED) { + node._flags &= ~SUBSCRIBED; - oldDeps.clear(); - oldDeps = prevOldDeps; - currentSignal = prevSignal; - }; + const prev = node._prevTarget; + const next = node._nextTarget; + if (prev !== undefined) { + prev._nextTarget = next; + node._prevTarget = undefined; + } + if (next !== undefined) { + next._prevTarget = prev; + node._nextTarget = undefined; + } + if (node === this._targets) { + this._targets = next; + } + } } subscribe(fn: (value: T) => void): () => void { return effect(() => fn(this.value)); } - /** - * A custom update routine to run when this Signal's value changes. - * @internal - */ - _updater() { - // override me to handle updates + toString(): string { + return this.value + ""; + } + + peek(): T { + return this._value as T; } -} -function mark(signal: Signal) { - if (signal._pending++ === 0) { - signal._subs.forEach(mark); + get value(): T { + return getValue(this); + } + + set value(value: T) { + if (value !== this._value) { + if (batchIteration > 100) { + cycleDetected(); + } + + this._value = value; + this._version++; + globalVersion++; + + /**@__INLINE__*/ startBatch(); + try { + for ( + let node = this._targets; + node !== undefined; + node = node._nextTarget + ) { + node._target._notify(); + } + } finally { + endBatch(); + } + } } } -function unmark(signal: Signal) { - // We can only unmark this node as not needing an update if it - // wasn't flagged as needing an update by someone else. This is - // done to make the sweeping logic independent of the order - // in which a dependency tries to unmark a subtree. - if ( - !signal._requiresUpdate && - signal._pending > 0 && - --signal._pending === 0 +export function signal(value: T): Signal { + return new Signal(value); +} + +function prepareSources(target: Computed | Effect) { + for ( + let node = target._sources; + node !== undefined; + node = node._nextSource ) { - signal._subs.forEach(unmark); + const rollbackNode = node._source._node; + if (rollbackNode !== undefined) { + node._rollbackNode = rollbackNode; + } + node._source._node = node; + node._flags |= STALE; } } -function sweep(subs: Set>) { - subs.forEach(signal => { - // If a computed errored during sweep, we'll discard that subtree - // for this sweep cycle by setting PENDING to 0; - if (signal._pending > 1) return --signal._pending; - let ready = true; - signal._deps.forEach(dep => { - if (dep._pending > 0) ready = false; - }); - - if (ready && signal._pending > 0 && --signal._pending === 0) { - if (signal._isComputing) { - throw Error("Cycle detected"); +function cleanupSources(target: Computed | Effect) { + // At this point target._sources is a mishmash of current & former dependencies. + // The current dependencies are also in a reverse order of use. + // Therefore build a new, reverted list of dependencies containing only the current + // dependencies in a proper order of use. + // Drop former dependencies from the list and unsubscribe from their change notifications. + + let node = target._sources; + let sources = undefined; + while (node !== undefined) { + const next = node._nextSource; + if (node._flags & STALE) { + node._source._unsubscribe(node); + node._nextSource = undefined; + } else { + if (sources !== undefined) { + sources._prevSource = node; } + node._prevSource = undefined; + node._nextSource = sources; + sources = node; + } - signal._requiresUpdate = false; - signal._isComputing = true; - signal._updater(); - signal._isComputing = false; - sweep(signal._subs); + node._source._node = node._rollbackNode; + if (node._rollbackNode !== undefined) { + node._rollbackNode = undefined; } - }); + node = next; + } + target._sources = sources; } -function subscribe(signal: Signal, to: Signal) { - signal._active = true; - signal._deps.add(to); - to._subs.add(signal); +function returnComputed(computed: Computed): T { + computed._flags &= ~RUNNING; + if (computed._flags & HAS_ERROR) { + throw computed._value; + } + return computed._value as T; } -function unsubscribe(signal: Signal, from: Signal) { - signal._deps.delete(from); - from._subs.delete(signal); - - // If nobody listens to the signal we depended on, we can traverse - // upwards and destroy all subscriptions until we encounter a writable - // signal or a signal that others listen to as well. - if (from._subs.size === 0) { - from._active = false; - from._deps.forEach(dep => unsubscribe(from, dep)); +function disposeNestedEffects(context: Computed | Effect) { + let effect = context._effects; + if (effect !== undefined) { + do { + effect._dispose(); + effect = effect._nextNestedEffect; + } while (effect !== undefined); + context._effects = undefined; } } -const tmpPending: Signal[] = []; -/** - * Refresh _just_ this signal and its dependencies recursively. - * All other signals will be left untouched and added to the - * global queue to flush later. Since we're traversing "upwards", - * we don't have to care about topological sorting. - */ -function refreshStale(signal: Signal) { - if (batchPending) { - batchPending.delete(signal); +class Computed extends Signal { + /** @internal */ + _compute: () => T; + + /** @internal */ + _sources?: Node = undefined; + + /** @internal */ + _effects?: Effect = undefined; + + /** @internal */ + _globalVersion = globalVersion - 1; + + /** @internal */ + _flags = STALE; + + constructor(compute: () => T) { + super(undefined); + this._compute = compute; } - signal._pending = 0; - signal._updater(); - if (commitError) { - const err = commitError; - commitError = null; - throw err; + /** @internal */ + _subscribe(node: Node) { + if (this._targets === undefined) { + this._flags |= STALE | SHOULD_SUBSCRIBE; + + // A computed signal subscribes lazily to its dependencies when the it + // gets its first subscriber. + for ( + let node = this._sources; + node !== undefined; + node = node._nextSource + ) { + node._source._subscribe(node); + } + } + super._subscribe(node); } - signal._subs.forEach(sub => { - if (sub._pending > 0) { - // If PENDING > 1 then we can safely reduce the counter because - // the final sweep will take care of the rest. But if it's - // exactly 1 we can't do that otherwise the sweeping logic - // assumes that this signal was already updated. - if (sub._pending > 1) sub._pending--; - tmpPending.push(sub); + /** @internal */ + _unsubscribe(node: Node) { + super._unsubscribe(node); + + // Computed signal unsubscribes from its dependencies from it loses its last subscriber. + if (this._targets === undefined) { + this._flags &= ~SHOULD_SUBSCRIBE; + + for ( + let node = this._sources; + node !== undefined; + node = node._nextSource + ) { + node._source._unsubscribe(node); + } } - }); -} + } -function activate(signal: Signal) { - signal._active = true; - refreshStale(signal); -} + /** @internal */ + _notify() { + if (!(this._flags & NOTIFIED)) { + this._flags |= STALE | NOTIFIED; + + for ( + let node = this._targets; + node !== undefined; + node = node._nextTarget + ) { + node._target._notify(); + } + } + } -export function signal(value: T): Signal { - return new Signal(value); -} + peek(): T { + this._flags &= ~NOTIFIED; -export type ReadonlySignal = Omit, "value"> & { - readonly value: T; -}; -export function computed(compute: () => T): ReadonlySignal { - const signal = new Signal(undefined as any); - signal._readonly = true; + if (this._flags & RUNNING) { + cycleDetected(); + } + this._flags |= RUNNING; + + if (!(this._flags & STALE) && this._targets !== undefined) { + return returnComputed(this); + } + this._flags &= ~STALE; + + if (this._globalVersion === globalVersion) { + return returnComputed(this); + } + this._globalVersion = globalVersion; + + if (this._version > 0) { + // Check current dependencies for changes. The dependency list is already in + // order of use. Therefore if >1 dependencies have changed only the first used one + // is re-evaluated at this point. + let node = this._sources; + while (node !== undefined) { + if (node._source._version !== node._version) { + break; + } + try { + node._source.peek(); + } catch { + // Failures of current dependencies shouldn't be rethrown here in case the + // compute function catches them. + } + if (node._source._version !== node._version) { + break; + } + node = node._nextSource; + } + if (node === undefined) { + return returnComputed(this); + } + } - function updater() { - let finish = signal._setCurrent(); + disposeNestedEffects(this); + const prevValue = this._value; + const prevFlags = this._flags; + const prevContext = evalContext; try { - let ret = compute(); - const stale = signal._value === ret; - if (!stale) signal._subs.forEach(sub => (sub._requiresUpdate = true)); - finish(stale, true); - signal._value = ret; - } catch (err: any) { - // Ensure that we log the first error not the last - if (!commitError) commitError = err; - finish(true, false); + evalContext = this; + prepareSources(this); + this._value = this._compute(); + this._flags &= ~HAS_ERROR; + if ( + prevFlags & HAS_ERROR || + this._value !== prevValue || + this._version === 0 + ) { + this._version++; + } + } catch (err) { + this._value = err; + this._flags |= HAS_ERROR; + this._version++; + } finally { + cleanupSources(this); + evalContext = prevContext; } + return returnComputed(this); } - signal._updater = updater; + get value(): T { + if (this._flags & RUNNING) { + cycleDetected(); + } + return getValue(this); + } +} - return signal; +export function computed(compute: () => T): Computed { + return new Computed(compute); } +export type { Computed as ReadonlySignal }; + +function endEffect(this: Effect, prevContext?: Computed | Effect) { + if (evalContext !== this) { + throw new Error("Out-of-order effect"); + } -export function effect(callback: () => void) { - const s = computed(() => batch(callback)); - // Set up subscriptions since this is a "reactor" signal - activate(s); - return () => s._setCurrent()(true, true); + cleanupSources(this); + + evalContext = prevContext; + endBatch(); + + this._flags &= ~RUNNING; } -export function batch(cb: () => T): T { - if (batchPending !== null) { - return cb(); - } else { - const pending: Set = new Set(); +class Effect { + _compute: () => void; + _sources?: Node; + _effects?: Effect; + _nextNestedEffect?: Effect; + _nextBatchedEffect?: Effect; + _flags = SHOULD_SUBSCRIBE; - batchPending = pending; + constructor(compute: () => void) { + this._compute = compute; + } + _callback() { + const finish = this._start(); try { - return cb(); + this._compute(); } finally { - // Since stale signals are refreshed upwards, we need to - // add pending signals in reverse - let item: Signal | undefined; - while ((item = tmpPending.pop()) !== undefined) { - pending.add(item); - } + finish(); + } + } - batchPending = null; + _start() { + if (this._flags & RUNNING) { + cycleDetected(); + } + this._flags |= RUNNING; + this._flags &= ~DISPOSED; + disposeNestedEffects(this); + + /*@__INLINE__**/ startBatch(); + const prevContext = evalContext; + if (prevContext !== undefined) { + this._nextNestedEffect = prevContext._effects; + prevContext._effects = this; + } + evalContext = this; - sweep(pending); - if (commitError) { - const err = commitError; - // Clear global error flag for next commit - commitError = null; - throw err; - } + prepareSources(this); + return endEffect.bind(this, prevContext); + } + + _notify() { + if (!(this._flags & NOTIFIED)) { + this._flags |= NOTIFIED; + this._nextBatchedEffect = batchedEffect; + batchedEffect = this; } } + + _dispose() { + if (this._flags & RUNNING) { + throw new Error("Effect still running"); + } + for ( + let node = this._sources; + node !== undefined; + node = node._nextSource + ) { + node._source._unsubscribe(node); + } + disposeNestedEffects(this); + this._sources = undefined; + this._flags |= DISPOSED; + } +} + +export function effect(compute: () => void): () => void { + const effect = new Effect(compute); + effect._callback(); + // Return a bound function instead of a wrapper like `() => effect._dispose()`, + // because bound functions seem to be just as fast and take up a lot less memory. + return effect._dispose.bind(effect); } diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index e64f8aa9e..c1d10b640 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -1,4 +1,4 @@ -import { signal, computed, effect, batch } from "@preact/signals-core"; +import { signal, computed, effect, batch, Signal } from "@preact/signals-core"; describe("signal", () => { it("should return value", () => { @@ -151,7 +151,7 @@ describe("effect()", () => { const fn = () => effect(() => { // Prevent test suite from spinning if limit is not hit - if (i++ > 10) { + if (i++ > 200) { throw new Error("test failed"); } a.value; @@ -160,6 +160,281 @@ describe("effect()", () => { expect(fn).to.throw(/Cycle detected/); }); + + it("should automatically dispose nested effects on re-evaluation", () => { + const a = signal(0); + const b = signal(0); + const spyInner = sinon.spy(() => a.value + b.value); + const spyOuter = sinon.spy(() => { + a.value; + effect(spyInner); + }); + effect(spyOuter); + + expect(spyOuter).to.be.calledOnce; + expect(spyInner).to.be.calledOnce; + spyOuter.resetHistory(); + spyInner.resetHistory(); + + a.value = 1; + expect(spyOuter).to.be.calledOnce; + expect(spyInner).to.be.calledOnce; + spyOuter.resetHistory(); + spyInner.resetHistory(); + + b.value = 2; + expect(spyOuter).to.not.be.called; + expect(spyInner).to.be.calledOnce; + }); + + it("should automatically dispose nested effects on disposal", () => { + const a = signal(0); + const b = signal(0); + const spyInner = sinon.spy(() => a.value + b.value); + const spyOuter = sinon.spy(() => { + a.value; + effect(spyInner); + }); + const dispose = effect(spyOuter); + + expect(spyOuter).to.be.calledOnce; + expect(spyInner).to.be.calledOnce; + spyOuter.resetHistory(); + spyInner.resetHistory(); + + dispose(); + + a.value = 1; + expect(spyOuter).not.to.be.called; + expect(spyInner).not.to.be.called; + }); + + it("should keep nested computed signals active even after enclosing effect", () => { + const a = signal(0); + const spy = sinon.spy(() => a.value); + + let c!: Signal; + const dispose = effect(() => { + c = computed(() => { + a.value; + effect(spy); + }); + }); + dispose(); + expect(spy).not.to.be.called; + + c.value; + expect(spy).to.be.calledOnce; + a.value = 1; + expect(spy).to.be.calledTwice; + }); + + it("should allow disposing the effect multiple times", () => { + const dispose = effect(() => undefined); + dispose(); + expect(() => dispose()).to.throw; + }); + + it("should throw when disposing a running effect", () => { + const a = signal(0); + const dispose = effect(() => { + if (a.value === 1) { + dispose(); + } + }); + expect(() => { + a.value = 1; + }).to.throw("Effect still running"); + }); + + it("should not run if it's first been triggered and then disposed in a batch", () => { + const a = signal(0); + const spy = sinon.spy(() => a.value); + const dispose = effect(spy); + spy.resetHistory(); + + batch(() => { + a.value = 1; + dispose(); + }); + + expect(spy).not.to.be.called; + }); + + it("should not run if it's been triggered, disposed and then triggered again in a batch", () => { + const a = signal(0); + const spy = sinon.spy(() => a.value); + const dispose = effect(spy); + spy.resetHistory(); + + batch(() => { + a.value = 2; + dispose(); + }); + + expect(spy).not.to.be.called; + }); + + // Test internal behavior depended on by Preact & React integrations + describe("internals", () => { + it("should pass in the effect instance in callback's `this`", () => { + let e: any; + effect(function (this: any) { e = this; }); + expect(e).to.have.property("_start"); + expect(e).to.have.property("_dispose"); + }); + + it("should allow setting _callback that replaces the default functionality", () => { + const a = signal(0); + const oldSpy = sinon.spy(); + const newSpy = sinon.spy(); + + let e: any; + effect(function (this: any) { + e = this; + a.value; + oldSpy(); + }); + oldSpy.resetHistory(); + + e._callback = newSpy; + a.value = 1; + + expect(oldSpy).not.to.be.called; + expect(newSpy).to.be.called; + }); + + it("should returns a function for closing the effect scope from _start", () => { + const s = signal(0); + + let e: any; + effect(function (this: any) { e = this; }); + + const spy = sinon.spy(); + e._callback = spy; + + const done1 = e._start(); + s.value; + done1(); + expect(spy).not.to.be.called; + + s.value = 2; + expect(spy).to.be.called; + spy.resetHistory(); + + const done2 = e._start(); + done2(); + + s.value = 3; + expect(spy).not.to.be.called; + }); + + it("should throw on out-of-order start1-start2-end1 sequences", () => { + let e1: any; + effect(function (this: any) { e1 = this; }); + + let e2: any; + effect(function (this: any) { e2 = this; }); + + const done1 = e1._start(); + const done2 = e2._start() + try { + expect(() => done1()).to.throw(/Out-of-order/); + } finally { + done2() + done1(); + } + }); + + it("should throw a cycle detection error when _start is called while the effect is running", () => { + let e: any; + effect(function (this: any) { e = this; }); + + const done = e._start(); + try { + expect(() => e._start()).to.throw(/Cycle detected/); + } finally { + done(); + } + }); + + it("should dispose the effect on _dispose", () => { + const s = signal(0); + + let e: any; + effect(function (this: any) { e = this; }); + + const spy = sinon.spy(); + e._callback = spy; + + const done = e._start(); + try { + s.value; + } finally { + done(); + } + expect(spy).not.to.be.called; + + s.value = 2; + expect(spy).to.be.called; + spy.resetHistory(); + + e._dispose(); + s.value = 2; + expect(spy).not.to.be.called; + }); + + it("should allow reusing the effect after disposing it", () => { + const s = signal(0); + + let e: any; + effect(function (this: any) { e = this; }); + + const spy = sinon.spy(); + e._callback = spy; + e._dispose(); + + const done = e._start(); + try { + s.value; + } finally { + done(); + } + s.value = 2; + expect(spy).to.be.called; + }); + + it("should have property _sources that is undefined when and only when the effect has no sources", () => { + const s = signal(0); + + let e: any; + effect(function (this: any) { e = this; }); + expect(e._sources).to.be.undefined; + + const done1 = e._start(); + try { + s.value; + } finally { + done1(); + } + expect(e._sources).not.to.be.undefined; + + const done2 = e._start(); + done2(); + expect(e._sources).to.be.undefined; + + const done3 = e._start(); + try { + s.value; + } finally { + done3(); + } + expect(e._sources).not.to.be.undefined; + + e._dispose(); + expect(e._sources).to.be.undefined; + }); + }); }); describe("computed()", () => { @@ -182,6 +457,101 @@ describe("computed()", () => { expect(c.value).to.equal("aab"); }); + it("should be lazily computed on demand", () => { + const a = signal("a"); + const b = signal("b"); + const spy = sinon.spy(() => a.value + b.value); + const c = computed(spy); + expect(spy).to.not.be.called; + c.value; + expect(spy).to.be.calledOnce; + a.value = "x"; + b.value = "y"; + expect(spy).to.be.calledOnce; + c.value; + expect(spy).to.be.calledTwice; + }); + + it("should computed only when dependency has changed at some point", () => { + const a = signal("a"); + const spy = sinon.spy(() => { + return a.value; + }); + const c = computed(spy); + c.value; + expect(spy).to.be.calledOnce; + a.value = "a"; + c.value; + expect(spy).to.be.calledOnce; + }); + + it("should detect simple dependency cycles", () => { + const a: Signal = computed(() => a.value); + expect(() => a.value).to.throw(/Cycle detected/); + }); + + it("should automatically dispose nested effects on re-evaluation", () => { + const a = signal(0); + const b = signal(0); + const spyInner = sinon.spy(() => a.value + b.value); + const spyOuter = sinon.spy(() => { + a.value; + effect(spyInner); + }); + const c = computed(spyOuter); + c.value; + expect(spyOuter).to.be.calledOnce; + expect(spyInner).to.be.calledOnce; + spyOuter.resetHistory(); + spyInner.resetHistory(); + + a.value = 1; + expect(spyOuter).not.to.be.called; + expect(spyInner).to.be.calledOnce; + spyOuter.resetHistory(); + spyInner.resetHistory(); + + c.value; + expect(spyOuter).to.be.calledOnce; + expect(spyInner).to.be.calledOnce; + }); + + it("should not allow a computed signal to become a direct dependency of itself", () => { + const spy = sinon.spy(() => { + try { + a.value; + } catch { + // pass + } + }); + const a = computed(spy); + a.value; + expect(() => effect(() => a.value)).to.not.throw(); + }); + + it("should detect deep dependency cycles", () => { + const a: Signal = computed(() => b.value); + const b: Signal = computed(() => c.value); + const c: Signal = computed(() => d.value); + const d: Signal = computed(() => a.value); + expect(() => a.value).to.throw(/Cycle detected/); + }); + + it("should store failures and recompute after a dependency changes", () => { + const a = signal(0); + const spy = sinon.spy(() => { + a.value; + throw new Error(); + }); + const c = computed(spy); + expect(() => c.value).to.throw(); + expect(() => c.value).to.throw(); + expect(spy).to.be.calledOnce; + a.value = 1; + expect(() => c.value).to.throw(); + expect(spy).to.be.calledTwice; + }); + it("should conditionally unsubscribe from signals", () => { const a = signal("a"); const b = signal("b"); @@ -210,6 +580,88 @@ describe("computed()", () => { expect(spy).not.to.be.called; }); + it("should consider undefined value separate from uninitialized value", () => { + const a = signal(0); + const spy = sinon.spy(() => undefined); + const c = computed(spy); + + expect(c.value).to.be.undefined; + a.value = 1; + expect(c.value).to.be.undefined; + expect(spy).to.be.calledOnce; + }); + + it("should not leak errors raised by dependencies", () => { + const a = signal(0); + const b = computed(() => { + a.value; + throw new Error("error"); + }); + const c = computed(() => { + try { + b.value; + } catch { + return "ok"; + } + }); + expect(c.value).to.equal("ok"); + a.value = 1; + expect(c.value).to.equal("ok"); + }); + + it("should propagate invalidation even right after first subscription", () => { + const a = signal(0); + const b = computed(() => a.value); + const c = computed(() => b.value); + c.value; + + const spy = sinon.spy(() => { + c.value; + }); + + effect(spy); + expect(spy).to.be.calledOnce; + spy.resetHistory(); + + a.value = 1; + expect(spy).to.be.calledOnce; + }); + + it("should not recompute dependencies out of order", () => { + const a = signal(1); + const b = signal(1); + const c = signal(1); + + const spy = sinon.spy(() => c.value); + const d = computed(spy); + + const e = computed(() => { + if (a.value > 0) { + b.value; + d.value; + } else { + b.value; + } + }); + + e.value; + spy.resetHistory(); + + a.value = 2; + b.value = 2; + c.value = 2; + e.value; + expect(spy).to.be.calledOnce; + spy.resetHistory(); + + a.value = -1; + b.value = -1; + c.value = -1; + e.value; + expect(spy).not.to.be.called; + spy.resetHistory(); + }); + describe("graph updates", () => { it("should run computeds once for multiple dep changes", async () => { const a = signal("a"); @@ -226,6 +678,8 @@ describe("computed()", () => { compute.resetHistory(); a.value = "aa"; + b.value = "bb"; + c.value; expect(compute).to.have.been.calledOnce; }); @@ -251,7 +705,7 @@ describe("computed()", () => { compute.resetHistory(); a.value = 4; - + d.value; expect(compute).to.have.been.calledOnce; }); @@ -467,6 +921,7 @@ describe("computed()", () => { spy.resetHistory(); a.value = "aa"; + d.value; expect(spy).to.returned("aa c"); }); @@ -495,31 +950,17 @@ describe("computed()", () => { spy.resetHistory(); a.value = "aa"; + e.value; expect(spy).to.returned("aa c d"); }); - - it("should prevent invalid unmark state when called on a source signal", () => { - // Don't allow our internal logic to get in an invalid state, even through - // our own internal API. The bug this tests for is that a source signal - // will be unmarked, leading to all its subscribers `_pending` value to become - // negative. This is invalid and breaks further updates. - const a = signal("a"); - const b = computed(() => a.value); - effect(() => b.value); - - a._setCurrent()(true, true); - - a.value = "aa"; - expect(b.value).to.equal("aa"); - }); }); describe("error handling", () => { it("should throw when writing to computeds", () => { const a = signal("a"); const b = computed(() => a.value); - const fn = () => ((b as any).value = "aa"); - expect(fn).to.throw(/readonly/); + const fn = () => ((b as Signal).value = "aa"); + expect(fn).to.throw(/Cannot set property value/); }); it("should keep graph consistent on errors during activation", () => { @@ -536,70 +977,20 @@ describe("computed()", () => { it("should keep graph consistent on errors in computeds", () => { const a = signal(0); - let shouldThrow = false; const b = computed(() => { - if (shouldThrow) throw new Error("fail"); + if (a.value === 1) throw new Error("fail"); return a.value; }); const c = computed(() => b.value); expect(c.value).to.equal(0); - shouldThrow = true; - let error: Error | null = null; - try { - a.value = 1; - } catch (err: any) { - error = err; - } - expect(error?.message).to.equal("fail"); + a.value = 1; + expect(() => b.value).to.throw("fail"); - // Now update signal again without throwing an error. If we didn't - // reset the subtree's PENDING counter C's value wouldn't update. - shouldThrow = false; a.value = 2; expect(c.value).to.equal(2); }); - it("should revert subscriptions on errors in computeds", () => { - const a = signal(1); - const b = signal(1); - const c = signal(1); - let shouldThrow = false; - const compute = sinon.spy(() => { - if (shouldThrow) { - throw new Error("fail: " + c.value); - } - return a.value + b.value; - }); - const d = computed(compute); - expect(d.value).to.equal(2); - - shouldThrow = true; - expect(() => { - a.value = 2; - }).to.throw(); - expect(d.value).to.equal(2); - - // when errors occur, we intentionally over-subscribe. - // This includes retaining subscriptions after the error: - compute.resetHistory(); - try { - b.value = 2; - } catch (e) { - // may error, but not in a way we can assert over - } - expect(compute).to.have.been.called; - - compute.resetHistory(); - shouldThrow = false; - // Note: b.value=2 should probably also update the subgraph. - // ...but its value is already 2 from the errored computation. - // b.value = 2; - c.value = 2; - expect(compute).to.have.been.called; - expect(d.value).to.equal(4); - }); - it("should support lazy branches", () => { const a = signal(0); const b = computed(() => a.value); diff --git a/packages/preact/src/index.ts b/packages/preact/src/index.ts index 47ee32ccc..3be8a149d 100644 --- a/packages/preact/src/index.ts +++ b/packages/preact/src/index.ts @@ -13,8 +13,9 @@ import { ComponentType, OptionsTypes, HookFn, - Updater, - ElementUpdater, + Effect, + PropertyEffect, + AugmentedElement as Element, } from "./internal"; export { signal, computed, batch, effect, Signal, type ReadonlySignal }; @@ -35,22 +36,25 @@ function hook(hookName: T, hookFn: HookFn) { } let currentComponent: Component | undefined; -let currentUpdater: Updater | undefined; -let finishUpdate: ReturnType | undefined; -const updaterForComponent = new WeakMap(); +let currentUpdater: Effect | undefined; +let finishUpdate: (() => void) | undefined; +const updaterForComponent = new WeakMap(); -function setCurrentUpdater(updater?: Updater) { +function setCurrentUpdater(updater?: Effect) { // end tracking for the current update: - if (finishUpdate) finishUpdate(true, true); + if (finishUpdate) finishUpdate(); // start tracking the new update: currentUpdater = updater; - finishUpdate = updater && updater._setCurrent(); + finishUpdate = updater && updater._start(); } -function createUpdater(updater: () => void) { - const s = signal(undefined) as Updater; - s._updater = updater; - return s; +function createUpdater(update: () => void) { + let updater!: Effect; + effect(function (this: Effect) { + updater = this; + }); + updater._callback = update; + return updater; } /** @todo This may be needed for complex prop value detection. */ @@ -85,8 +89,8 @@ function Text(this: ComponentType, { data }: { data: Signal }) { } // Replace this component's vdom updater with a direct text one: - currentUpdater!._updater = () => { - (this.base as Text).data = s._value; + currentUpdater!._callback = () => { + (this.base as Text).data = s.peek(); }; return computed(() => { @@ -172,55 +176,57 @@ hook(OptionsTypes.DIFFED, (old, vnode) => { currentComponent = undefined; let dom: Element; - let updater: ElementUpdater; // vnode._dom is undefined during string rendering, // so we use this to skip prop subscriptions during SSR. if (typeof vnode.type === "string" && (dom = vnode.__e as Element)) { let props = vnode.__np; if (props) { - // @ts-ignore-next - updater = dom._updater; - if (!updater) { - updater = createElementUpdater(dom); - // @ts-ignore-next - dom._updater = updater; + let updaters = dom._updaters; + if (updaters) { + for (let prop in updaters) { + let updater = updaters[prop]; + if (updater !== undefined && !(prop in props)) { + updater._dispose(); + // @todo we could just always invoke _dispose() here + updaters[prop] = undefined; + } + } + } else { + updaters = {}; + dom._updaters = updaters; + } + for (let prop in props) { + let updater = updaters[prop]; + let signal = props[prop]; + if (updater === undefined) { + updater = createPropUpdater(dom, prop, signal); + updaters[prop] = updater; + } + setCurrentUpdater(updater); + updater._callback(signal); } - updater!._props = props; - setCurrentUpdater(updater); - // @ts-ignore-next we're adding an argument here - updater._updater(true); } } old(vnode); }); -// per-element updater for 1+ signal bindings -function createElementUpdater(dom: Element) { - const cache: Record = { __proto__: null }; - const updater = createUpdater((skip?: boolean) => { - const props = updater._props; - for (let prop in props) { - if (prop === "children") continue; - let signal = props[prop]; - if (signal instanceof Signal) { - let value = signal.value; - let cached = cache[prop]; - cache[prop] = value; - if (skip === true || cached === value) { - // this is just a subscribe run, not an update - } else if (prop in dom) { - // @ts-ignore-next-line silly - dom[prop] = value; - } else if (value) { - dom.setAttribute(prop, value); - } else { - dom.removeAttribute(prop); - } - } +function createPropUpdater(dom: Element, prop: string, signal: Signal) { + const setAsProperty = prop in dom; + return createUpdater((newSignal?: Signal) => { + if (newSignal) signal = newSignal; + let value = signal.value; + if (newSignal) { + // just a new signal reference passed in, don't update + } else if (setAsProperty) { + // @ts-ignore-next-line silly + dom[prop] = value; + } else if (value) { + dom.setAttribute(prop, value); + } else { + dom.removeAttribute(prop); } - }) as ElementUpdater; - return updater; + }) as PropertyEffect; } /** Unsubscribe from Signals when unmounting components/vnodes */ @@ -229,18 +235,19 @@ hook(OptionsTypes.UNMOUNT, (old, vnode: VNode) => { const updater = component && updaterForComponent.get(component); if (updater) { updaterForComponent.delete(component); - updater._setCurrent()(true, true); + updater._dispose(); } if (typeof vnode.type === "string") { const dom = vnode.__e as Element; - // @ts-ignore-next - const updater = dom._updater; - if (updater) { - updater._setCurrent()(true, true); - // @ts-ignore-next - dom._updater = null; + const updaters = dom._updaters; + if (updaters) { + dom._updaters = null; + for (let prop in updaters) { + let updater = updaters[prop]; + if (updater) updater._dispose(); + } } } old(vnode); @@ -260,7 +267,7 @@ Component.prototype.shouldComponentUpdate = function (props, state) { // @todo: Once preactjs/preact#3671 lands, this could just use `currentUpdater`: const updater = updaterForComponent.get(this); - const hasSignals = updater && updater._deps?.size !== 0; + const hasSignals = updater && updater._sources !== undefined; // let reason; // if (!hasSignals && !hasComputeds.has(this)) { diff --git a/packages/preact/src/internal.d.ts b/packages/preact/src/internal.d.ts index ce8c688a3..6b9de1baa 100644 --- a/packages/preact/src/internal.d.ts +++ b/packages/preact/src/internal.d.ts @@ -1,6 +1,21 @@ import { Component } from "preact"; import { Signal } from "@preact/signals-core"; +export interface Effect { + _sources: object | undefined; + _start(): () => void; + _callback(): void; + _dispose(): void; +} + +export interface PropertyEffect extends Effect { + _callback(newSignal?: Signal): void; +} + +export interface AugmentedElement extends HTMLElement { + _updaters?: Record | null; +} + export interface VNode

extends preact.VNode

{ /** The component instance for this VNode */ __c: Component; @@ -17,12 +32,6 @@ export interface ComponentType extends Component { __v: VNode; } -export type Updater = Signal; - -export interface ElementUpdater extends Updater { - _props: Record; -} - export const enum OptionsTypes { HOOK = "__h", DIFF = "__b", diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 27cf22def..a42900ad0 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -14,7 +14,7 @@ import { Signal, type ReadonlySignal, } from "@preact/signals-core"; -import { Updater, ReactOwner, ReactDispatcher } from "./internal"; +import { Effect, ReactOwner, ReactDispatcher } from "./internal"; export { signal, computed, batch, effect, Signal, type ReadonlySignal }; @@ -53,20 +53,23 @@ function createPropUpdater(props: any, prop: string, signal: Signal) { } */ -let finishUpdate: ReturnType | undefined; -const updaterForComponent = new WeakMap(); +let finishUpdate: (() => void) | undefined; +const updaterForComponent = new WeakMap(); -function setCurrentUpdater(updater?: Updater) { +function setCurrentUpdater(updater?: Effect) { // end tracking for the current update: - if (finishUpdate) finishUpdate(true, true); + if (finishUpdate) finishUpdate(); // start tracking the new update: - finishUpdate = updater && updater._setCurrent(); + finishUpdate = updater && updater._start(); } -function createUpdater(updater: () => void) { - const s = signal(undefined) as Updater; - s._updater = updater; - return s; +function createUpdater(update: () => void) { + let updater!: Effect; + effect(function (this: Effect) { + updater = this; + }); + updater._callback = update; + return updater; } /** @@ -127,7 +130,7 @@ Object.defineProperty(internals.ReactCurrentDispatcher, "current", { updater = createUpdater(rerender); updaterForComponent.set(lastOwner, updater); } else { - updater._updater = rerender; + updater._callback = rerender; } setCurrentUpdater(updater); } else { diff --git a/packages/react/src/internal.d.ts b/packages/react/src/internal.d.ts index 9bfad5b8c..24e283359 100644 --- a/packages/react/src/internal.d.ts +++ b/packages/react/src/internal.d.ts @@ -1,5 +1,12 @@ import { Signal } from "@preact/signals-core"; +export interface Effect { + _sources: object | undefined; + _start(): () => void; + _callback(): void; + _dispose(): void; +} + export interface ReactOwner { _: never; }