From f923b76064e860efd749577224930fb021b35e0b Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Tue, 13 Sep 2022 17:05:51 +0300 Subject: [PATCH 01/66] Implement core without Sets, Maps, Arrays etc. --- packages/core/src/index.ts | 519 +++++++++++++++++++++---------------- 1 file changed, 290 insertions(+), 229 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 9d41b915c..2e94bee67 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -1,295 +1,356 @@ -/** This tracks subscriptions of signals read inside a computed */ -let currentSignal: Signal | undefined; -let commitError: Error | null = null; - -let batchPending: Set | null = null; - -let oldDeps = new Set(); +// 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 source whose value the target depends on. + signal: Signal; + nextSignal?: 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; +}; -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 addTargetToAllSources(target: Computed | Effect) { + for (let node = target._sources; node; node = node.nextSignal) { + node.signal._subscribe(node); } +} - toString() { - return "" + this.value; +function removeTargetFromAllSources(target: Computed | Effect) { + for (let node = target._sources; node; node = node.nextSignal) { + node.signal._unsubscribe(node); } +} - peek() { - if (!this._active || this._pending > 0) { - activate(this); - } - return this._value; +type RollbackItem = { + signal: Signal; + currentTarget?: Computed | Effect | undefined; + next?: RollbackItem; +}; + +function rollback(item: RollbackItem | undefined) { + for (let rollback = item; rollback; rollback = rollback.next) { + rollback.signal._currentTarget = rollback.currentTarget; } +} - get value() { - if (!this._active || this._pending > 0) { - activate(this); - } +type BatchItem = { + effect: Effect; + next?: BatchItem; +}; + +function startBatch() { + batchDepth++; +} + +function endBatch() { + if (--batchDepth === 0) { + const batch = currentBatch; + currentBatch = undefined; - // 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; + for (let item = batch; item; item = item.next) { + const runnable = item.effect; + runnable._batched = false; + runnable._run() } + } +} - // subscribe the current computed to this signal: - this._subs.add(currentSignal); - // update the current computed's dependencies: - currentSignal._deps.add(this); - oldDeps.delete(this); +export function batch(callback: () => T): T { + if (batchDepth > 0) { + return callback(); + } + /*@__INLINE__**/ startBatch(); + try { + return callback(); + } finally { + endBatch(); + } +} - return this._value; +// A list for rolling back source's ._currentTarget values after a target has been evaluated. +let currentRollback: RollbackItem | undefined = undefined; +// Currently evaluated computed or effect. +let currentTarget: Computed | Effect | undefined = undefined; +// Effects collected into a batch. +let currentBatch: BatchItem | undefined = undefined; +let batchDepth = 0; +// A global version number for signalss, 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 (currentTarget !== undefined && signal._currentTarget !== currentTarget) { + node = { signal: signal, target: currentTarget, version: 0 }; + currentRollback = { signal: signal, currentTarget: signal._currentTarget, next: currentRollback }; + signal._currentTarget = currentTarget; + } + const value = signal.peek(); + if (currentTarget && node) { + node.nextSignal = currentTarget._sources; + node.version = node.signal._version; + currentTarget._sources = node; } + return value; +} - set value(value) { - if (this._readonly) { - throw Error("Computed signals are readonly"); - } +export class Signal { + /** @internal */ + _value: unknown; - if (this._value !== value) { - this._value = value; + /** @internal */ + _version = 0; - batch(() => { - batchPending!.add(this); + /** @internal */ + _currentTarget?: Computed | Effect = undefined; - // in batch mode this signal may be marked already - if (this._pending === 0) { - mark(this); - } - }); + /** @internal */ + _targets?: Node = undefined; + + constructor(value?: T) { + this._value = value; + } + + /** @internal */ + _subscribe(node: Node) { + if (this._targets) { + this._targets.prevTarget = node; } + node.nextTarget = this._targets; + node.prevTarget = undefined; + this._targets = node; } - /** - * 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 */ + _unsubscribe(node: Node) { + const prev = node.prevTarget; + const next = node.nextTarget; + node.prevTarget = undefined; + node.nextTarget = undefined; - oldDeps.clear(); - oldDeps = prevOldDeps; - currentSignal = prevSignal; - }; + if (prev) { + prev.nextTarget = next; + } + if (next) { + next.prevTarget = prev; + } + if (node === this._targets) { + this._targets = next; + } } - subscribe(fn: (value: T) => void): () => void { - return effect(() => fn(this.value)); + toString(): string { + return "" + this.value; } - /** - * A custom update routine to run when this Signal's value changes. - * @internal - */ - _updater() { - // override me to handle updates + 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); } -} -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 - ) { - signal._subs.forEach(unmark); - } -} + set value(value: T) { + if (value !== this._value) { + this._value = value; + this._version++; + globalVersion++; -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"); + /**@__INLINE__*/ startBatch(); + for (let node = this._targets; node; node = node.nextTarget) { + node.target._invalidate(); } - - signal._requiresUpdate = false; - signal._isComputing = true; - signal._updater(); - signal._isComputing = false; - sweep(signal._subs); + endBatch(); } - }); + } } -function subscribe(signal: Signal, to: Signal) { - signal._active = true; - signal._deps.add(to); - to._subs.add(signal); +export function signal(value: T): Signal { + return new Signal(value); } -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 returnComputed(computed: Computed): T { + computed._valid = true; + computed._globalVersion = globalVersion; + if (computed._valueIsError) { + throw computed._value; } + return computed._value as T; } -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); +export class Computed extends Signal{ + _compute?: () => T; + _sources?: Node = undefined;; + _computing = false; + _valid = false; + _valueIsError = false; + _globalVersion = globalVersion - 1; + + constructor(compute: () => T) { + super(undefined); + this._compute = compute; } - signal._pending = 0; - signal._updater(); - if (commitError) { - const err = commitError; - commitError = null; - throw err; + _subscribe(node: Node) { + if (!this._targets) { + // A computed signal subscribes lazily to its dependencies when + // the computed signal gets its first subscriber. + this._valid = false; + addTargetToAllSources(this); + } + 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); + _unsubscribe(node: Node) { + super._unsubscribe(node); + + // When a computed signal loses its last subscriber it also unsubscribes + // from its own dependencies. + if (!this._targets) { + removeTargetFromAllSources(this); } - }); -} + } -function activate(signal: Signal) { - signal._active = true; - refreshStale(signal); -} + _invalidate() { + this._valid = false; + for (let node = this._targets; node; node = node.nextTarget) { + node.target._invalidate(); + } + } -export function signal(value: T): Signal { - return new Signal(value); -} + peek(): T { + if (this._computing) { + throw new Error("cycle detected"); + } + if (this._globalVersion === globalVersion) { + return returnComputed(this); + } + if (this._targets && this._valid) { + return returnComputed(this); + } -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._version > 0) { + let node = this._sources; + while (node) { + node.signal.peek(); + if (node.signal._version !== node.version) { + break; + } + node = node.nextSignal; + } + if (!node) { + return returnComputed(this); + } + } - function updater() { - let finish = signal._setCurrent(); + let value: unknown = undefined; + let valueIsError = false; + const prevContext = currentTarget; + const prevRollback = currentRollback; 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); + currentTarget = this; + currentRollback = undefined; + + removeTargetFromAllSources(this); + this._computing = true; + this._sources = undefined; + + value = this._compute!(); + } catch (err: unknown) { + valueIsError = true; + value = err; + } finally { + if (this._targets) { + addTargetToAllSources(this); + } + rollback(currentRollback); + this._computing = false; + currentTarget = prevContext; + currentRollback = prevRollback; + } + + if (valueIsError || this._valueIsError || this._value !== value) { + this._value = value; + this._valueIsError = valueIsError; + this._version++; } + return returnComputed(this); } - signal._updater = updater; + get value(): T { + return getValue(this); + } +} - return signal; +export interface ReadonlySignal extends Signal { + readonly value: T; } -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); +export function computed(compute: () => T): ReadonlySignal { + return new Computed(compute); } -export function batch(cb: () => T): T { - if (batchPending !== null) { - return cb(); - } else { - const pending: Set = new Set(); +class Effect { + _sources?: Node = undefined; + _batched = false; - batchPending = pending; + constructor(readonly _callback: () => void) { } + _run() { + const prevContext = currentTarget; + const prevRollback = currentRollback; try { - return cb(); + currentTarget = this; + currentRollback = undefined; + + removeTargetFromAllSources(this); + this._sources = undefined; + this._callback(); } 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); - } + addTargetToAllSources(this); + rollback(currentRollback); - batchPending = null; + currentTarget = prevContext; + currentRollback = prevRollback; + } + } - sweep(pending); - if (commitError) { - const err = commitError; - // Clear global error flag for next commit - commitError = null; - throw err; - } + _invalidate() { + if (!this._batched) { + this._batched = true; + currentBatch = { effect: this, next: currentBatch }; } } + + _dispose() { + for (let node = this._sources; node; node = node.nextSignal) { + node.signal._unsubscribe(node); + } + this._sources = undefined; + } +} + +export function effect(callback: () => void): () => void { + const effect = new Effect(callback); + effect._run(); + return effect._dispose.bind(effect); +} + +export function _doNotUseOrYouWillBeFired_notify(signal: S, callback: (signal: S) => void): () => void { + const cb = () => callback(signal); + const notify = new Effect(cb); + const node = { signal: signal as Signal, target: notify, version: 0 }; + notify._run = cb; + notify._sources = node; + (signal as Signal)._subscribe(node); + return notify._dispose.bind(notify); } From 7e7c9dc3521c99ad0326ed0d1aa71df5766d466f Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Tue, 13 Sep 2022 17:45:19 +0300 Subject: [PATCH 02/66] Rename currentTarget to evalContext --- packages/core/src/index.ts | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 2e94bee67..967b2a2ff 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -30,13 +30,13 @@ function removeTargetFromAllSources(target: Computed | Effect) { type RollbackItem = { signal: Signal; - currentTarget?: Computed | Effect | undefined; + evalContext?: Computed | Effect | undefined; next?: RollbackItem; }; function rollback(item: RollbackItem | undefined) { for (let rollback = item; rollback; rollback = rollback.next) { - rollback.signal._currentTarget = rollback.currentTarget; + rollback.signal._evalContext = rollback.evalContext; } } @@ -74,10 +74,10 @@ export function batch(callback: () => T): T { } } -// A list for rolling back source's ._currentTarget values after a target has been evaluated. +// A list for rolling back source's ._evalContext values after a target has been evaluated. let currentRollback: RollbackItem | undefined = undefined; // Currently evaluated computed or effect. -let currentTarget: Computed | Effect | undefined = undefined; +let evalContext: Computed | Effect | undefined = undefined; // Effects collected into a batch. let currentBatch: BatchItem | undefined = undefined; let batchDepth = 0; @@ -87,16 +87,16 @@ let globalVersion = 0; function getValue(signal: Signal): T { let node: Node | undefined = undefined; - if (currentTarget !== undefined && signal._currentTarget !== currentTarget) { - node = { signal: signal, target: currentTarget, version: 0 }; - currentRollback = { signal: signal, currentTarget: signal._currentTarget, next: currentRollback }; - signal._currentTarget = currentTarget; + if (evalContext !== undefined && signal._evalContext !== evalContext) { + node = { signal: signal, target: evalContext, version: 0 }; + currentRollback = { signal: signal, evalContext: signal._evalContext, next: currentRollback }; + signal._evalContext = evalContext; } const value = signal.peek(); - if (currentTarget && node) { - node.nextSignal = currentTarget._sources; + if (evalContext && node) { + node.nextSignal = evalContext._sources; node.version = node.signal._version; - currentTarget._sources = node; + evalContext._sources = node; } return value; } @@ -109,7 +109,7 @@ export class Signal { _version = 0; /** @internal */ - _currentTarget?: Computed | Effect = undefined; + _evalContext?: Computed | Effect = undefined; /** @internal */ _targets?: Node = undefined; @@ -254,10 +254,10 @@ export class Computed extends Signal{ let value: unknown = undefined; let valueIsError = false; - const prevContext = currentTarget; + const prevContext = evalContext; const prevRollback = currentRollback; try { - currentTarget = this; + evalContext = this; currentRollback = undefined; removeTargetFromAllSources(this); @@ -274,7 +274,7 @@ export class Computed extends Signal{ } rollback(currentRollback); this._computing = false; - currentTarget = prevContext; + evalContext = prevContext; currentRollback = prevRollback; } @@ -306,10 +306,10 @@ class Effect { constructor(readonly _callback: () => void) { } _run() { - const prevContext = currentTarget; + const prevContext = evalContext; const prevRollback = currentRollback; try { - currentTarget = this; + evalContext = this; currentRollback = undefined; removeTargetFromAllSources(this); @@ -319,7 +319,7 @@ class Effect { addTargetToAllSources(this); rollback(currentRollback); - currentTarget = prevContext; + evalContext = prevContext; currentRollback = prevRollback; } } From 06d3c9cfd9da25d524f79bd29ffffd9563f6c8ab Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Tue, 13 Sep 2022 18:01:52 +0300 Subject: [PATCH 03/66] Add a comment explaining why effect returns bound functions --- packages/core/src/index.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 967b2a2ff..2a8898227 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -342,6 +342,9 @@ class Effect { export function effect(callback: () => void): () => void { const effect = new Effect(callback); effect._run(); + + // 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); } From d61016da6474fb7867adda822edee80747090649 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Tue, 13 Sep 2022 18:11:42 +0300 Subject: [PATCH 04/66] Add Signal.subscribe back --- packages/core/src/index.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 2a8898227..f394d4fe6 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -146,6 +146,10 @@ export class Signal { } } + subscribe(fn: (value: T) => void): () => void { + return effect(() => fn(this.value)); + } + toString(): string { return "" + this.value; } From ec3467daaa658ae729c0901537d06c3ae515c8f4 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Tue, 13 Sep 2022 18:18:57 +0300 Subject: [PATCH 05/66] Code cleanup --- packages/core/src/index.ts | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index f394d4fe6..7a661ad19 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -57,7 +57,7 @@ function endBatch() { for (let item = batch; item; item = item.next) { const runnable = item.effect; runnable._batched = false; - runnable._run() + runnable._run(); } } } @@ -89,7 +89,11 @@ function getValue(signal: Signal): T { let node: Node | undefined = undefined; if (evalContext !== undefined && signal._evalContext !== evalContext) { node = { signal: signal, target: evalContext, version: 0 }; - currentRollback = { signal: signal, evalContext: signal._evalContext, next: currentRollback }; + currentRollback = { + signal: signal, + evalContext: signal._evalContext, + next: currentRollback, + }; signal._evalContext = evalContext; } const value = signal.peek(); @@ -190,9 +194,9 @@ function returnComputed(computed: Computed): T { return computed._value as T; } -export class Computed extends Signal{ +export class Computed extends Signal { _compute?: () => T; - _sources?: Node = undefined;; + _sources?: Node = undefined; _computing = false; _valid = false; _valueIsError = false; @@ -352,12 +356,15 @@ export function effect(callback: () => void): () => void { return effect._dispose.bind(effect); } -export function _doNotUseOrYouWillBeFired_notify(signal: S, callback: (signal: S) => void): () => void { +export function _doNotUseOrYouWillBeFired_notify( + signal: S, + callback: (signal: S) => void +): () => void { const cb = () => callback(signal); const notify = new Effect(cb); - const node = { signal: signal as Signal, target: notify, version: 0 }; + const node = { signal: signal, target: notify, version: 0 }; notify._run = cb; notify._sources = node; - (signal as Signal)._subscribe(node); + signal._subscribe(node); return notify._dispose.bind(notify); } From c914029140ba13346bacce280f12dd52e7d42c8b Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Tue, 13 Sep 2022 18:45:00 +0300 Subject: [PATCH 06/66] Unsubscribe from dropped dependencies only after subscribing to new ones --- packages/core/src/index.ts | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 7a661ad19..f9f2051a1 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -16,14 +16,14 @@ type Node = { version: number; }; -function addTargetToAllSources(target: Computed | Effect) { - for (let node = target._sources; node; node = node.nextSignal) { +function subscribeToAll(sources: Node | undefined) { + for (let node = sources; node; node = node.nextSignal) { node.signal._subscribe(node); } } -function removeTargetFromAllSources(target: Computed | Effect) { - for (let node = target._sources; node; node = node.nextSignal) { +function unsubscribeFromAll(sources: Node | undefined) { + for (let node = sources; node; node = node.nextSignal) { node.signal._unsubscribe(node); } } @@ -212,8 +212,9 @@ export class Computed extends Signal { // A computed signal subscribes lazily to its dependencies when // the computed signal gets its first subscriber. this._valid = false; - addTargetToAllSources(this); + subscribeToAll(this._sources); } + super._subscribe(node); } @@ -223,14 +224,16 @@ export class Computed extends Signal { // When a computed signal loses its last subscriber it also unsubscribes // from its own dependencies. if (!this._targets) { - removeTargetFromAllSources(this); + unsubscribeFromAll(this._sources); } } _invalidate() { - this._valid = false; - for (let node = this._targets; node; node = node.nextTarget) { - node.target._invalidate(); + if (this._valid) { + this._valid = false; + for (let node = this._targets; node; node = node.nextTarget) { + node.target._invalidate(); + } } } @@ -262,13 +265,13 @@ export class Computed extends Signal { let value: unknown = undefined; let valueIsError = false; + const oldSources = this._sources; const prevContext = evalContext; const prevRollback = currentRollback; try { evalContext = this; currentRollback = undefined; - removeTargetFromAllSources(this); this._computing = true; this._sources = undefined; @@ -278,8 +281,9 @@ export class Computed extends Signal { value = err; } finally { if (this._targets) { - addTargetToAllSources(this); + subscribeToAll(this._sources); } + unsubscribeFromAll(oldSources); rollback(currentRollback); this._computing = false; evalContext = prevContext; @@ -314,17 +318,17 @@ class Effect { constructor(readonly _callback: () => void) { } _run() { + const oldSources = this._sources; const prevContext = evalContext; const prevRollback = currentRollback; try { evalContext = this; currentRollback = undefined; - - removeTargetFromAllSources(this); this._sources = undefined; this._callback(); } finally { - addTargetToAllSources(this); + subscribeToAll(this._sources); + unsubscribeFromAll(oldSources); rollback(currentRollback); evalContext = prevContext; @@ -362,9 +366,9 @@ export function _doNotUseOrYouWillBeFired_notify( ): () => void { const cb = () => callback(signal); const notify = new Effect(cb); - const node = { signal: signal, target: notify, version: 0 }; + const node = { signal: signal as Signal, target: notify, version: 0 }; notify._run = cb; notify._sources = node; - signal._subscribe(node); + (signal as Signal)._subscribe(node); return notify._dispose.bind(notify); } From 3a37f2e4cf5815ce57136fe5ab79f5b4ed790819 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Tue, 13 Sep 2022 19:43:02 +0300 Subject: [PATCH 07/66] Fix computed invalidation --- packages/core/src/index.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index f9f2051a1..86c3a923d 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -229,11 +229,9 @@ export class Computed extends Signal { } _invalidate() { - if (this._valid) { - this._valid = false; - for (let node = this._targets; node; node = node.nextTarget) { - node.target._invalidate(); - } + this._valid = false; + for (let node = this._targets; node; node = node.nextTarget) { + node.target._invalidate(); } } @@ -369,6 +367,6 @@ export function _doNotUseOrYouWillBeFired_notify( const node = { signal: signal as Signal, target: notify, version: 0 }; notify._run = cb; notify._sources = node; - (signal as Signal)._subscribe(node); + signal._subscribe(node); return notify._dispose.bind(notify); } From 44d91d4a794e4f682a40c6372dcecac4a1531ba5 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Tue, 13 Sep 2022 20:18:23 +0300 Subject: [PATCH 08/66] Expose _doNotUseOrYouWillBeFired_Capture --- packages/core/src/index.ts | 93 ++++++++++++++++++++------------------ 1 file changed, 50 insertions(+), 43 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 86c3a923d..0f7034020 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -6,7 +6,7 @@ type Node = { nextSignal?: Node; // A target that depends on the source and should be notified when the source changes. - target: Computed | Effect; + target: Computed | Capture; prevTarget?: Node; nextTarget?: Node; @@ -30,7 +30,7 @@ function unsubscribeFromAll(sources: Node | undefined) { type RollbackItem = { signal: Signal; - evalContext?: Computed | Effect | undefined; + evalContext?: Computed | Capture | undefined; next?: RollbackItem; }; @@ -41,7 +41,7 @@ function rollback(item: RollbackItem | undefined) { } type BatchItem = { - effect: Effect; + capture: Capture; next?: BatchItem; }; @@ -55,9 +55,9 @@ function endBatch() { currentBatch = undefined; for (let item = batch; item; item = item.next) { - const runnable = item.effect; + const runnable = item.capture; runnable._batched = false; - runnable._run(); + runnable._notify(); } } } @@ -76,9 +76,9 @@ export function batch(callback: () => T): T { // A list for rolling back source's ._evalContext values after a target has been evaluated. let currentRollback: RollbackItem | undefined = undefined; -// Currently evaluated computed or effect. -let evalContext: Computed | Effect | undefined = undefined; -// Effects collected into a batch. +// Currently evaluated computed or capture. +let evalContext: Computed | Capture | undefined = undefined; +// Captures collected into a batch. let currentBatch: BatchItem | undefined = undefined; let batchDepth = 0; // A global version number for signalss, used for fast-pathing repeated @@ -113,7 +113,7 @@ export class Signal { _version = 0; /** @internal */ - _evalContext?: Computed | Effect = undefined; + _evalContext?: Computed | Capture = undefined; /** @internal */ _targets?: Node = undefined; @@ -309,39 +309,51 @@ export function computed(compute: () => T): ReadonlySignal { return new Computed(compute); } -class Effect { +class Capture { + /** @internal */ + _notify: () => void; + + /** @internal */ _sources?: Node = undefined; + + /** @internal */ _batched = false; - constructor(readonly _callback: () => void) { } + constructor(notify: () => void) { + this._notify = notify; + } - _run() { + start() { const oldSources = this._sources; const prevContext = evalContext; const prevRollback = currentRollback; - try { - evalContext = this; - currentRollback = undefined; - this._sources = undefined; - this._callback(); - } finally { - subscribeToAll(this._sources); - unsubscribeFromAll(oldSources); - rollback(currentRollback); - evalContext = prevContext; - currentRollback = prevRollback; - } + evalContext = this; + currentRollback = undefined; + this._sources = undefined; + return this._end.bind(this, oldSources, prevContext, prevRollback); + } + + /** @internal */ + _end(oldSources?: Node, prevContext?: Computed | Capture, prevRollback?: RollbackItem) { + subscribeToAll(this._sources); + unsubscribeFromAll(oldSources); + rollback(currentRollback); + + evalContext = prevContext; + currentRollback = prevRollback; } + /** @internal */ _invalidate() { if (!this._batched) { this._batched = true; - currentBatch = { effect: this, next: currentBatch }; + currentBatch = { capture: this, next: currentBatch }; } } - _dispose() { + /** @internal */ + dispose() { for (let node = this._sources; node; node = node.nextSignal) { node.signal._unsubscribe(node); } @@ -350,23 +362,18 @@ class Effect { } export function effect(callback: () => void): () => void { - const effect = new Effect(callback); - effect._run(); - - // Return a bound function instead of a wrapper like `() => effect._dispose()`, + const capture = new Capture(() => { + const finish = capture.start(); + try { + callback(); + } finally { + finish(); + } + }); + capture._notify(); + // Return a bound function instead of a wrapper like `() => capture._dispose()`, // because bound functions seem to be just as fast and take up a lot less memory. - return effect._dispose.bind(effect); + return capture.dispose.bind(capture); } -export function _doNotUseOrYouWillBeFired_notify( - signal: S, - callback: (signal: S) => void -): () => void { - const cb = () => callback(signal); - const notify = new Effect(cb); - const node = { signal: signal as Signal, target: notify, version: 0 }; - notify._run = cb; - notify._sources = node; - signal._subscribe(node); - return notify._dispose.bind(notify); -} +export { Capture as _doNotUseOrYouWillBeFired_Capture }; From 3e93e94f6202186c545de5aea26b62ca9309bbbe Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Tue, 13 Sep 2022 21:34:04 +0300 Subject: [PATCH 09/66] Expose Effect instance in effect callback's `this` --- packages/core/src/index.ts | 48 +++++++++++++++----------------------- 1 file changed, 19 insertions(+), 29 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 0f7034020..8f77554a3 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -6,7 +6,7 @@ type Node = { nextSignal?: Node; // A target that depends on the source and should be notified when the source changes. - target: Computed | Capture; + target: Computed | Effect; prevTarget?: Node; nextTarget?: Node; @@ -30,7 +30,7 @@ function unsubscribeFromAll(sources: Node | undefined) { type RollbackItem = { signal: Signal; - evalContext?: Computed | Capture | undefined; + evalContext?: Computed | Effect | undefined; next?: RollbackItem; }; @@ -41,7 +41,7 @@ function rollback(item: RollbackItem | undefined) { } type BatchItem = { - capture: Capture; + effect: Effect; next?: BatchItem; }; @@ -55,7 +55,7 @@ function endBatch() { currentBatch = undefined; for (let item = batch; item; item = item.next) { - const runnable = item.capture; + const runnable = item.effect; runnable._batched = false; runnable._notify(); } @@ -76,9 +76,9 @@ export function batch(callback: () => T): T { // A list for rolling back source's ._evalContext values after a target has been evaluated. let currentRollback: RollbackItem | undefined = undefined; -// Currently evaluated computed or capture. -let evalContext: Computed | Capture | undefined = undefined; -// Captures collected into a batch. +// Currently evaluated computed or effect. +let evalContext: Computed | Effect | undefined = undefined; +// Effects collected into a batch. let currentBatch: BatchItem | undefined = undefined; let batchDepth = 0; // A global version number for signalss, used for fast-pathing repeated @@ -113,7 +113,7 @@ export class Signal { _version = 0; /** @internal */ - _evalContext?: Computed | Capture = undefined; + _evalContext?: Computed | Effect = undefined; /** @internal */ _targets?: Node = undefined; @@ -309,21 +309,16 @@ export function computed(compute: () => T): ReadonlySignal { return new Computed(compute); } -class Capture { - /** @internal */ +class Effect { _notify: () => void; - - /** @internal */ _sources?: Node = undefined; - - /** @internal */ _batched = false; constructor(notify: () => void) { this._notify = notify; } - start() { + _start() { const oldSources = this._sources; const prevContext = evalContext; const prevRollback = currentRollback; @@ -334,8 +329,7 @@ class Capture { return this._end.bind(this, oldSources, prevContext, prevRollback); } - /** @internal */ - _end(oldSources?: Node, prevContext?: Computed | Capture, prevRollback?: RollbackItem) { + _end(oldSources?: Node, prevContext?: Computed | Effect, prevRollback?: RollbackItem) { subscribeToAll(this._sources); unsubscribeFromAll(oldSources); rollback(currentRollback); @@ -344,16 +338,14 @@ class Capture { currentRollback = prevRollback; } - /** @internal */ _invalidate() { if (!this._batched) { this._batched = true; - currentBatch = { capture: this, next: currentBatch }; + currentBatch = { effect: this, next: currentBatch }; } } - /** @internal */ - dispose() { + _dispose() { for (let node = this._sources; node; node = node.nextSignal) { node.signal._unsubscribe(node); } @@ -362,18 +354,16 @@ class Capture { } export function effect(callback: () => void): () => void { - const capture = new Capture(() => { - const finish = capture.start(); + const effect = new Effect(() => { + const finish = effect._start(); try { - callback(); + callback.call(effect); } finally { finish(); } }); - capture._notify(); - // Return a bound function instead of a wrapper like `() => capture._dispose()`, + effect._notify(); + // 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 capture.dispose.bind(capture); + return effect._dispose.bind(effect); } - -export { Capture as _doNotUseOrYouWillBeFired_Capture }; From de1de41207761ce925e13b1218b260e710234bb0 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Tue, 13 Sep 2022 22:09:04 +0300 Subject: [PATCH 10/66] Fix, make effects implicit batches --- packages/core/src/index.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 8f77554a3..0fbab7562 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -319,6 +319,7 @@ class Effect { } _start() { + /*@__INLINE__**/ startBatch(); const oldSources = this._sources; const prevContext = evalContext; const prevRollback = currentRollback; @@ -336,6 +337,7 @@ class Effect { evalContext = prevContext; currentRollback = prevRollback; + endBatch(); } _invalidate() { From 91a4ed98fae2babdfa10499306877307969f7597 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Tue, 13 Sep 2022 23:29:49 +0300 Subject: [PATCH 11/66] Test fixes --- packages/core/src/index.ts | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 0fbab7562..7bdef48ea 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -84,6 +84,8 @@ let batchDepth = 0; // A global version number for signalss, used for fast-pathing repeated // computed.peek()/computed.value calls when nothing has changed globally. let globalVersion = 0; +// FIXME +let effectDepth = 0; function getValue(signal: Signal): T { let node: Node | undefined = undefined; @@ -95,12 +97,16 @@ function getValue(signal: Signal): T { next: currentRollback, }; signal._evalContext = evalContext; + + if (effectDepth > 0) { + signal._subscribe(node); + } } const value = signal.peek(); if (evalContext && node) { node.nextSignal = evalContext._sources; - node.version = node.signal._version; evalContext._sources = node; + node.version = node.signal._version; } return value; } @@ -123,7 +129,10 @@ export class Signal { } /** @internal */ - _subscribe(node: Node) { + _subscribe(node: Node): void { + if (this._targets === node || node.prevTarget) { + return; + } if (this._targets) { this._targets.prevTarget = node; } @@ -133,12 +142,11 @@ export class Signal { } /** @internal */ - _unsubscribe(node: Node) { + _unsubscribe(node: Node): void { const prev = node.prevTarget; const next = node.nextTarget; node.prevTarget = undefined; node.nextTarget = undefined; - if (prev) { prev.nextTarget = next; } @@ -214,24 +222,24 @@ export class Computed extends Signal { this._valid = false; subscribeToAll(this._sources); } - super._subscribe(node); } _unsubscribe(node: Node) { - super._unsubscribe(node); - // When a computed signal loses its last subscriber it also unsubscribes // from its own dependencies. if (!this._targets) { unsubscribeFromAll(this._sources); } + super._unsubscribe(node) } _invalidate() { - this._valid = false; - for (let node = this._targets; node; node = node.nextTarget) { - node.target._invalidate(); + if (this._valid) { + this._valid = false; + for (let node = this._targets; node; node = node.nextTarget) { + node.target._invalidate(); + } } } @@ -323,6 +331,7 @@ class Effect { const oldSources = this._sources; const prevContext = evalContext; const prevRollback = currentRollback; + effectDepth++; evalContext = this; currentRollback = undefined; @@ -331,6 +340,7 @@ class Effect { } _end(oldSources?: Node, prevContext?: Computed | Effect, prevRollback?: RollbackItem) { + effectDepth--; subscribeToAll(this._sources); unsubscribeFromAll(oldSources); rollback(currentRollback); From 8c05a1486a1959f588306dbfabf96decf2550729 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Tue, 13 Sep 2022 23:41:06 +0300 Subject: [PATCH 12/66] Throw correct error when trying to set computed.value --- packages/core/src/index.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 7bdef48ea..405f4c6ca 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -245,7 +245,7 @@ export class Computed extends Signal { peek(): T { if (this._computing) { - throw new Error("cycle detected"); + throw new Error("Cycle detected"); } if (this._globalVersion === globalVersion) { return returnComputed(this); @@ -307,6 +307,10 @@ export class Computed extends Signal { get value(): T { return getValue(this); } + + set value(value: T) { + throw Error("Computed signals are readonly"); + } } export interface ReadonlySignal extends Signal { From 4cc4edbdedcca8dadc38e816bb8925f404599d28 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Tue, 13 Sep 2022 23:54:40 +0300 Subject: [PATCH 13/66] Update a couple of tests to account for computed signal laziness --- packages/core/test/signal.test.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index e64f8aa9e..a2b7ef8df 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -226,6 +226,8 @@ describe("computed()", () => { compute.resetHistory(); a.value = "aa"; + b.value = "bb"; + c.value; expect(compute).to.have.been.calledOnce; }); @@ -251,7 +253,7 @@ describe("computed()", () => { compute.resetHistory(); a.value = 4; - + e.value; expect(compute).to.have.been.calledOnce; }); @@ -467,6 +469,7 @@ describe("computed()", () => { spy.resetHistory(); a.value = "aa"; + d.value; expect(spy).to.returned("aa c"); }); @@ -495,6 +498,7 @@ describe("computed()", () => { spy.resetHistory(); a.value = "aa"; + e.value; expect(spy).to.returned("aa c d"); }); From 5e8b2f4bf57874efb39206265892eaa9c2f7bda3 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Tue, 13 Sep 2022 23:59:27 +0300 Subject: [PATCH 14/66] Remove an obsolete core test --- packages/core/test/signal.test.tsx | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index a2b7ef8df..fe6c2449d 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -501,21 +501,6 @@ describe("computed()", () => { 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", () => { From a9380e6bebc943e9ae4f7b3ba40fb200eff5475a Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 00:23:53 +0300 Subject: [PATCH 15/66] Update core graph tests to assume computed signals are pure --- packages/core/test/signal.test.tsx | 62 +++--------------------------- 1 file changed, 6 insertions(+), 56 deletions(-) diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index fe6c2449d..fb2068508 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", () => { @@ -507,7 +507,7 @@ describe("computed()", () => { it("should throw when writing to computeds", () => { const a = signal("a"); const b = computed(() => a.value); - const fn = () => ((b as any).value = "aa"); + const fn = () => ((b as Signal).value = "aa"); expect(fn).to.throw(/readonly/); }); @@ -525,70 +525,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"); - - // 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 = 1; + expect(() => b.value).to.throw("fail"); + 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); From c7a43b4a251288af6661d359c1ad208e8d8ad214 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 00:44:06 +0300 Subject: [PATCH 16/66] Add cycle detection for effects --- packages/core/src/index.ts | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 405f4c6ca..ad6753922 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -50,16 +50,17 @@ function startBatch() { } function endBatch() { - if (--batchDepth === 0) { + while (batchDepth === 1 && currentBatch) { const batch = currentBatch; currentBatch = undefined; - for (let item = batch; item; item = item.next) { + for (let item: BatchItem | undefined = batch; item; item = item.next) { const runnable = item.effect; runnable._batched = false; runnable._notify(); } } + batchDepth--; } export function batch(callback: () => T): T { @@ -245,7 +246,7 @@ export class Computed extends Signal { peek(): T { if (this._computing) { - throw new Error("Cycle detected"); + throw new Error("cycle detected"); } if (this._globalVersion === globalVersion) { return returnComputed(this); @@ -325,12 +326,18 @@ class Effect { _notify: () => void; _sources?: Node = undefined; _batched = false; + _started = false; constructor(notify: () => void) { this._notify = notify; } _start() { + if (this._started) { + throw new Error("Cycle detected"); + } + this._started = true; + /*@__INLINE__**/ startBatch(); const oldSources = this._sources; const prevContext = evalContext; @@ -352,10 +359,14 @@ class Effect { evalContext = prevContext; currentRollback = prevRollback; endBatch(); + + this._started = false; } _invalidate() { - if (!this._batched) { + if (this._started) { + throw new Error("Cycle detected"); + } else if (!this._batched) { this._batched = true; currentBatch = { effect: this, next: currentBatch }; } From b635844f9f0ee972c820a0d86ed483558e884078 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 01:32:30 +0300 Subject: [PATCH 17/66] Fix remaining test --- packages/core/src/index.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index ad6753922..fe8454181 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -79,14 +79,13 @@ export function batch(callback: () => T): T { let currentRollback: RollbackItem | undefined = undefined; // Currently evaluated computed or effect. let evalContext: Computed | Effect | undefined = undefined; +let effectDepth = 0; // Effects collected into a batch. let currentBatch: BatchItem | undefined = undefined; let batchDepth = 0; // A global version number for signalss, used for fast-pathing repeated // computed.peek()/computed.value calls when nothing has changed globally. let globalVersion = 0; -// FIXME -let effectDepth = 0; function getValue(signal: Signal): T { let node: Node | undefined = undefined; @@ -182,10 +181,13 @@ export class Signal { globalVersion++; /**@__INLINE__*/ startBatch(); - for (let node = this._targets; node; node = node.nextTarget) { - node.target._invalidate(); + try { + for (let node = this._targets; node; node = node.nextTarget) { + node.target._invalidate(); + } + } finally { + endBatch(); } - endBatch(); } } } From 34f7433cb13b9da5cbe3e2e4cebab89ed668e48f Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 02:49:37 +0300 Subject: [PATCH 18/66] Throw a cycle error when the current batch handler hasn't settled after 100 iterations --- packages/core/src/index.ts | 42 +++++++++++++++++++++--------- packages/core/test/signal.test.tsx | 2 +- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index fe8454181..cc21530af 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -50,17 +50,38 @@ function startBatch() { } function endBatch() { - while (batchDepth === 1 && currentBatch) { + if (batchDepth > 1) { + batchDepth--; + return; + } + + let error: unknown; + let hasError = false; + + while (currentBatch) { const batch = currentBatch; currentBatch = undefined; + batchIteration++; for (let item: BatchItem | undefined = batch; item; item = item.next) { const runnable = item.effect; runnable._batched = false; - runnable._notify(); + try { + runnable._notify(); + } catch (err) { + if (!hasError) { + error = err; + hasError = true; + } + } } } + batchIteration = 0; batchDepth--; + + if (hasError) { + throw error; + } } export function batch(callback: () => T): T { @@ -83,6 +104,7 @@ let effectDepth = 0; // Effects collected into a batch. let currentBatch: BatchItem | undefined = undefined; let batchDepth = 0; +let batchIteration = 0; // A global version number for signalss, used for fast-pathing repeated // computed.peek()/computed.value calls when nothing has changed globally. let globalVersion = 0; @@ -176,6 +198,10 @@ export class Signal { set value(value: T) { if (value !== this._value) { + if (batchIteration > 100) { + throw new Error("Cycle detected"); + } + this._value = value; this._version++; globalVersion++; @@ -328,18 +354,12 @@ class Effect { _notify: () => void; _sources?: Node = undefined; _batched = false; - _started = false; constructor(notify: () => void) { this._notify = notify; } _start() { - if (this._started) { - throw new Error("Cycle detected"); - } - this._started = true; - /*@__INLINE__**/ startBatch(); const oldSources = this._sources; const prevContext = evalContext; @@ -361,14 +381,10 @@ class Effect { evalContext = prevContext; currentRollback = prevRollback; endBatch(); - - this._started = false; } _invalidate() { - if (this._started) { - throw new Error("Cycle detected"); - } else if (!this._batched) { + if (!this._batched) { this._batched = true; currentBatch = { effect: this, next: currentBatch }; } diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index fb2068508..6cba2a4fc 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -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; From a69540dfc022466ae6386ea36594c6fbd1a30f5e Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 03:09:14 +0300 Subject: [PATCH 19/66] Minor type cleanup --- packages/core/src/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index cc21530af..be6e42ed4 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -232,7 +232,7 @@ function returnComputed(computed: Computed): T { } export class Computed extends Signal { - _compute?: () => T; + _compute: () => T; _sources?: Node = undefined; _computing = false; _valid = false; @@ -310,7 +310,7 @@ export class Computed extends Signal { this._computing = true; this._sources = undefined; - value = this._compute!(); + value = this._compute(); } catch (err: unknown) { valueIsError = true; value = err; From f10c23f28f79c3052f6ac6e5bcad12d884e858b4 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 03:45:29 +0300 Subject: [PATCH 20/66] Optimize subscription logic --- packages/core/src/index.ts | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index be6e42ed4..7247f7954 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -16,12 +16,6 @@ type Node = { version: number; }; -function subscribeToAll(sources: Node | undefined) { - for (let node = sources; node; node = node.nextSignal) { - node.signal._subscribe(node); - } -} - function unsubscribeFromAll(sources: Node | undefined) { for (let node = sources; node; node = node.nextSignal) { node.signal._unsubscribe(node); @@ -100,7 +94,9 @@ export function batch(callback: () => T): T { let currentRollback: RollbackItem | undefined = undefined; // Currently evaluated computed or effect. let evalContext: Computed | Effect | undefined = undefined; -let effectDepth = 0; +// Used for keeping track whether the current evaluation context should automatically +// subscribe for updates to the signals depends on. +let subscribeDepth = 0; // Effects collected into a batch. let currentBatch: BatchItem | undefined = undefined; let batchDepth = 0; @@ -120,7 +116,7 @@ function getValue(signal: Signal): T { }; signal._evalContext = evalContext; - if (effectDepth > 0) { + if (subscribeDepth > 0) { signal._subscribe(node); } } @@ -249,7 +245,9 @@ export class Computed extends Signal { // A computed signal subscribes lazily to its dependencies when // the computed signal gets its first subscriber. this._valid = false; - subscribeToAll(this._sources); + for (let node = this._sources; node; node = node.nextSignal) { + node.signal._subscribe(node); + } } super._subscribe(node); } @@ -300,6 +298,7 @@ export class Computed extends Signal { let value: unknown = undefined; let valueIsError = false; + const targets = this._targets; const oldSources = this._sources; const prevContext = evalContext; const prevRollback = currentRollback; @@ -310,13 +309,18 @@ export class Computed extends Signal { this._computing = true; this._sources = undefined; + if (targets) { + // Computed signals with current targets should automatically subscribe to + // new dependencies it uses in the compute function. + subscribeDepth++; + } value = this._compute(); } catch (err: unknown) { valueIsError = true; value = err; } finally { - if (this._targets) { - subscribeToAll(this._sources); + if (targets) { + subscribeDepth--; } unsubscribeFromAll(oldSources); rollback(currentRollback); @@ -364,7 +368,7 @@ class Effect { const oldSources = this._sources; const prevContext = evalContext; const prevRollback = currentRollback; - effectDepth++; + subscribeDepth++; evalContext = this; currentRollback = undefined; @@ -373,8 +377,7 @@ class Effect { } _end(oldSources?: Node, prevContext?: Computed | Effect, prevRollback?: RollbackItem) { - effectDepth--; - subscribeToAll(this._sources); + subscribeDepth--; unsubscribeFromAll(oldSources); rollback(currentRollback); From 347ea41915c00621769dfefdd77a791fd5b52629 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 04:19:36 +0300 Subject: [PATCH 21/66] Avoid creating rollback items in trivial cases --- packages/core/src/index.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 7247f7954..eb12b7d66 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -24,7 +24,7 @@ function unsubscribeFromAll(sources: Node | undefined) { type RollbackItem = { signal: Signal; - evalContext?: Computed | Effect | undefined; + evalContext: Computed | Effect; next?: RollbackItem; }; @@ -109,11 +109,9 @@ function getValue(signal: Signal): T { let node: Node | undefined = undefined; if (evalContext !== undefined && signal._evalContext !== evalContext) { node = { signal: signal, target: evalContext, version: 0 }; - currentRollback = { - signal: signal, - evalContext: signal._evalContext, - next: currentRollback, - }; + if (signal._evalContext) { + currentRollback = { signal: signal, evalContext: signal._evalContext, next: currentRollback, }; + } signal._evalContext = evalContext; if (subscribeDepth > 0) { @@ -323,6 +321,9 @@ export class Computed extends Signal { subscribeDepth--; } unsubscribeFromAll(oldSources); + for (let node = this._sources; node; node = node.nextSignal) { + node.signal._evalContext = undefined; + } rollback(currentRollback); this._computing = false; evalContext = prevContext; @@ -379,6 +380,9 @@ class Effect { _end(oldSources?: Node, prevContext?: Computed | Effect, prevRollback?: RollbackItem) { subscribeDepth--; unsubscribeFromAll(oldSources); + for (let node = this._sources; node; node = node.nextSignal) { + node.signal._evalContext = undefined; + } rollback(currentRollback); evalContext = prevContext; From 9d3f884d7aa866c3491c0f3caf42e389087a1df2 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 04:43:27 +0300 Subject: [PATCH 22/66] Modify signal tracking logic --- packages/core/src/index.ts | 66 +++++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 25 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index eb12b7d66..f3eb440dd 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -24,13 +24,13 @@ function unsubscribeFromAll(sources: Node | undefined) { type RollbackItem = { signal: Signal; - evalContext: Computed | Effect; + node: Node; next?: RollbackItem; }; function rollback(item: RollbackItem | undefined) { for (let rollback = item; rollback; rollback = rollback.next) { - rollback.signal._evalContext = rollback.evalContext; + rollback.signal._node = rollback.node; } } @@ -90,7 +90,7 @@ export function batch(callback: () => T): T { } } -// A list for rolling back source's ._evalContext values after a target has been evaluated. +// A list for rolling back source's ._node values after a target has been evaluated. let currentRollback: RollbackItem | undefined = undefined; // Currently evaluated computed or effect. let evalContext: Computed | Effect | undefined = undefined; @@ -106,22 +106,24 @@ let batchIteration = 0; let globalVersion = 0; function getValue(signal: Signal): T { - let node: Node | undefined = undefined; - if (evalContext !== undefined && signal._evalContext !== evalContext) { - node = { signal: signal, target: evalContext, version: 0 }; - if (signal._evalContext) { - currentRollback = { signal: signal, evalContext: signal._evalContext, next: currentRollback, }; + let node = signal._node; + if (evalContext && (!node || node.target !== evalContext)) { + if (node) { + currentRollback = { signal: signal, node: node, next: currentRollback }; } - signal._evalContext = evalContext; + + node = { signal: signal, nextSignal: evalContext._sources, target: evalContext, version: 0 }; + evalContext._sources = node; + signal._node = node; if (subscribeDepth > 0) { signal._subscribe(node); } + } else { + node = undefined; } const value = signal.peek(); if (evalContext && node) { - node.nextSignal = evalContext._sources; - evalContext._sources = node; node.version = node.signal._version; } return value; @@ -135,7 +137,7 @@ export class Signal { _version = 0; /** @internal */ - _evalContext?: Computed | Effect = undefined; + _node?: Node = undefined; /** @internal */ _targets?: Node = undefined; @@ -303,29 +305,36 @@ export class Computed extends Signal { try { evalContext = this; currentRollback = undefined; - - this._computing = true; - this._sources = undefined; - if (targets) { // Computed signals with current targets should automatically subscribe to // new dependencies it uses in the compute function. subscribeDepth++; } + + this._sources = undefined; + this._computing = true; value = this._compute(); } catch (err: unknown) { valueIsError = true; value = err; } finally { - if (targets) { - subscribeDepth--; + this._computing = false; + + let node = oldSources; + while (node) { + const next = node.nextSignal; + node.signal._unsubscribe(node); + node.nextSignal = undefined; + node = next; } - unsubscribeFromAll(oldSources); for (let node = this._sources; node; node = node.nextSignal) { - node.signal._evalContext = undefined; + node.signal._node = undefined; } rollback(currentRollback); - this._computing = false; + + if (targets) { + subscribeDepth--; + } evalContext = prevContext; currentRollback = prevRollback; } @@ -369,22 +378,29 @@ class Effect { const oldSources = this._sources; const prevContext = evalContext; const prevRollback = currentRollback; - subscribeDepth++; evalContext = this; currentRollback = undefined; + subscribeDepth++; + this._sources = undefined; return this._end.bind(this, oldSources, prevContext, prevRollback); } _end(oldSources?: Node, prevContext?: Computed | Effect, prevRollback?: RollbackItem) { - subscribeDepth--; - unsubscribeFromAll(oldSources); + let node = oldSources; + while (node) { + const next = node.nextSignal; + node.signal._unsubscribe(node); + node.nextSignal = undefined; + node = next; + } for (let node = this._sources; node; node = node.nextSignal) { - node.signal._evalContext = undefined; + node.signal._node = undefined; } rollback(currentRollback); + subscribeDepth--; evalContext = prevContext; currentRollback = prevRollback; endBatch(); From 0aaba283759a5122e89589f6cd3f864b1d0d4e1f Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 05:09:04 +0300 Subject: [PATCH 23/66] Recycle linked list nodes --- packages/core/src/index.ts | 122 +++++++++++++++++++------------------ 1 file changed, 64 insertions(+), 58 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index f3eb440dd..1ce7c4ff5 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -14,13 +14,10 @@ type Node = { // 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; -}; -function unsubscribeFromAll(sources: Node | undefined) { - for (let node = sources; node; node = node.nextSignal) { - node.signal._unsubscribe(node); - } -} + // Whether the target is currently depending the signal. + used: boolean; +}; type RollbackItem = { signal: Signal; @@ -28,12 +25,6 @@ type RollbackItem = { next?: RollbackItem; }; -function rollback(item: RollbackItem | undefined) { - for (let rollback = item; rollback; rollback = rollback.next) { - rollback.signal._node = rollback.node; - } -} - type BatchItem = { effect: Effect; next?: BatchItem; @@ -106,24 +97,29 @@ let batchIteration = 0; let globalVersion = 0; function getValue(signal: Signal): T { - let node = signal._node; - if (evalContext && (!node || node.target !== evalContext)) { - if (node) { - currentRollback = { signal: signal, node: node, next: currentRollback }; - } + let node: Node | undefined = undefined; + if (evalContext) { + node = signal._node; + if (!node || node.target !== evalContext) { + if (node) { + currentRollback = { signal: signal, node: node, next: currentRollback }; + } - node = { signal: signal, nextSignal: evalContext._sources, target: evalContext, version: 0 }; - evalContext._sources = node; - signal._node = node; + node = { signal: signal, nextSignal: evalContext._sources, target: evalContext, version: 0, used: true }; + evalContext._sources = node; + signal._node = node; - if (subscribeDepth > 0) { - signal._subscribe(node); + if (node && subscribeDepth > 0) { + signal._subscribe(node); + } + } else if (!node.used) { + node.used = true; + } else { + node = undefined; } - } else { - node = undefined; } const value = signal.peek(); - if (evalContext && node) { + if (node) { node.version = node.signal._version; } return value; @@ -148,9 +144,6 @@ export class Signal { /** @internal */ _subscribe(node: Node): void { - if (this._targets === node || node.prevTarget) { - return; - } if (this._targets) { this._targets.prevTarget = node; } @@ -218,6 +211,39 @@ export function signal(value: T): Signal { return new Signal(value); } +function prepareSources(target: Computed | Effect) { + for (let node = target._sources; node; node = node.nextSignal) { + const signal = node.signal; + if (signal._node) { + currentRollback = { signal: signal, node: signal._node, next: currentRollback }; + } + signal._node = node; + node.used = false; + } +} + +function cleanupSources(target: Computed | Effect) { + let sources = undefined; + let node = target._sources; + while (node) { + const next = node.nextSignal; + if (node.used) { + node.nextSignal = sources; + sources = node; + } else { + node.signal._unsubscribe(node); + node.nextSignal = undefined; + } + node.signal._node = undefined; + node = next; + } + target._sources = sources; + + for (let rollback = currentRollback; rollback; rollback = rollback.next) { + rollback.signal._node = rollback.node; + } +} + function returnComputed(computed: Computed): T { computed._valid = true; computed._globalVersion = globalVersion; @@ -256,7 +282,9 @@ export class Computed extends Signal { // When a computed signal loses its last subscriber it also unsubscribes // from its own dependencies. if (!this._targets) { - unsubscribeFromAll(this._sources); + for (let node = this._sources; node; node = node.nextSignal) { + node.signal._unsubscribe(node); + } } super._unsubscribe(node) } @@ -299,7 +327,6 @@ export class Computed extends Signal { let valueIsError = false; const targets = this._targets; - const oldSources = this._sources; const prevContext = evalContext; const prevRollback = currentRollback; try { @@ -311,7 +338,8 @@ export class Computed extends Signal { subscribeDepth++; } - this._sources = undefined; + prepareSources(this); + this._computing = true; value = this._compute(); } catch (err: unknown) { @@ -320,18 +348,7 @@ export class Computed extends Signal { } finally { this._computing = false; - let node = oldSources; - while (node) { - const next = node.nextSignal; - node.signal._unsubscribe(node); - node.nextSignal = undefined; - node = next; - } - for (let node = this._sources; node; node = node.nextSignal) { - node.signal._node = undefined; - } - rollback(currentRollback); - + cleanupSources(this); if (targets) { subscribeDepth--; } @@ -375,7 +392,6 @@ class Effect { _start() { /*@__INLINE__**/ startBatch(); - const oldSources = this._sources; const prevContext = evalContext; const prevRollback = currentRollback; @@ -383,22 +399,12 @@ class Effect { currentRollback = undefined; subscribeDepth++; - this._sources = undefined; - return this._end.bind(this, oldSources, prevContext, prevRollback); + prepareSources(this); + return this._end.bind(this, prevContext, prevRollback); } - _end(oldSources?: Node, prevContext?: Computed | Effect, prevRollback?: RollbackItem) { - let node = oldSources; - while (node) { - const next = node.nextSignal; - node.signal._unsubscribe(node); - node.nextSignal = undefined; - node = next; - } - for (let node = this._sources; node; node = node.nextSignal) { - node.signal._node = undefined; - } - rollback(currentRollback); + _end(prevContext?: Computed | Effect, prevRollback?: RollbackItem) { + cleanupSources(this); subscribeDepth--; evalContext = prevContext; From d31bbf58b9180bdc736da38455391fe4e17e683f Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 06:39:24 +0300 Subject: [PATCH 24/66] Use Effect object themselves as batching linked list --- packages/core/src/index.ts | 41 +++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 1ce7c4ff5..d4ac557fd 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -25,11 +25,6 @@ type RollbackItem = { next?: RollbackItem; }; -type BatchItem = { - effect: Effect; - next?: BatchItem; -}; - function startBatch() { batchDepth++; } @@ -43,22 +38,25 @@ function endBatch() { let error: unknown; let hasError = false; - while (currentBatch) { - const batch = currentBatch; - currentBatch = undefined; + while (batchedEffect) { + let effect: Effect | undefined = batchedEffect; + batchedEffect = undefined; + batchIteration++; - for (let item: BatchItem | undefined = batch; item; item = item.next) { - const runnable = item.effect; - runnable._batched = false; + while (effect) { + const next: Effect | undefined = effect._nextEffect; + effect._nextEffect = undefined; + effect._batched = false; try { - runnable._notify(); + effect._notify(); } catch (err) { if (!hasError) { error = err; hasError = true; } } + effect = next; } } batchIteration = 0; @@ -89,7 +87,7 @@ let evalContext: Computed | Effect | undefined = undefined; // subscribe for updates to the signals depends on. let subscribeDepth = 0; // Effects collected into a batch. -let currentBatch: BatchItem | undefined = undefined; +let batchedEffect: Effect | undefined = undefined; let batchDepth = 0; let batchIteration = 0; // A global version number for signalss, used for fast-pathing repeated @@ -104,12 +102,11 @@ function getValue(signal: Signal): T { if (node) { currentRollback = { signal: signal, node: node, next: currentRollback }; } - node = { signal: signal, nextSignal: evalContext._sources, target: evalContext, version: 0, used: true }; evalContext._sources = node; signal._node = node; - if (node && subscribeDepth > 0) { + if (subscribeDepth > 0) { signal._subscribe(node); } } else if (!node.used) { @@ -118,11 +115,13 @@ function getValue(signal: Signal): T { node = undefined; } } - const value = signal.peek(); - if (node) { - node.version = node.signal._version; + try { + return signal.peek(); + } finally { + if (node) { + node.version = signal._version; + } } - return value; } export class Signal { @@ -385,6 +384,7 @@ class Effect { _notify: () => void; _sources?: Node = undefined; _batched = false; + _nextEffect?: Effect = undefined; constructor(notify: () => void) { this._notify = notify; @@ -415,7 +415,8 @@ class Effect { _invalidate() { if (!this._batched) { this._batched = true; - currentBatch = { effect: this, next: currentBatch }; + this._nextEffect = batchedEffect; + batchedEffect = this; } } From 805393a44a068da7c9a629dd462bb7e7f89f7190 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 09:16:00 +0300 Subject: [PATCH 25/66] Remove separate rollback buffer, use nodes to remember signal's previous ._node values --- packages/core/src/index.ts | 43 +++++++++++++------------------------- 1 file changed, 15 insertions(+), 28 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index d4ac557fd..d552c4dca 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -17,12 +17,10 @@ type Node = { // Whether the target is currently depending the signal. used: boolean; -}; -type RollbackItem = { - signal: Signal; - node: Node; - next?: RollbackItem; + // Used to remember & roll back signal's previous `._node` value when entering & exiting + // a new evaluation context. + rollbackNode?: Node; }; function startBatch() { @@ -79,8 +77,6 @@ export function batch(callback: () => T): T { } } -// A list for rolling back source's ._node values after a target has been evaluated. -let currentRollback: RollbackItem | undefined = undefined; // Currently evaluated computed or effect. let evalContext: Computed | Effect | undefined = undefined; // Used for keeping track whether the current evaluation context should automatically @@ -99,10 +95,7 @@ function getValue(signal: Signal): T { if (evalContext) { node = signal._node; if (!node || node.target !== evalContext) { - if (node) { - currentRollback = { signal: signal, node: node, next: currentRollback }; - } - node = { signal: signal, nextSignal: evalContext._sources, target: evalContext, version: 0, used: true }; + node = { signal: signal, nextSignal: evalContext._sources, target: evalContext, version: 0, used: true, rollbackNode: node }; evalContext._sources = node; signal._node = node; @@ -212,11 +205,11 @@ export function signal(value: T): Signal { function prepareSources(target: Computed | Effect) { for (let node = target._sources; node; node = node.nextSignal) { - const signal = node.signal; - if (signal._node) { - currentRollback = { signal: signal, node: signal._node, next: currentRollback }; + const rollbackNode = node.signal._node; + if (rollbackNode) { + node.rollbackNode = rollbackNode; } - signal._node = node; + node.signal._node = node; node.used = false; } } @@ -233,14 +226,14 @@ function cleanupSources(target: Computed | Effect) { node.signal._unsubscribe(node); node.nextSignal = undefined; } - node.signal._node = undefined; + + node.signal._node = node.rollbackNode; + if (node.rollbackNode) { + node.rollbackNode = undefined; + } node = next; } target._sources = sources; - - for (let rollback = currentRollback; rollback; rollback = rollback.next) { - rollback.signal._node = rollback.node; - } } function returnComputed(computed: Computed): T { @@ -327,10 +320,8 @@ export class Computed extends Signal { const targets = this._targets; const prevContext = evalContext; - const prevRollback = currentRollback; try { evalContext = this; - currentRollback = undefined; if (targets) { // Computed signals with current targets should automatically subscribe to // new dependencies it uses in the compute function. @@ -352,7 +343,6 @@ export class Computed extends Signal { subscribeDepth--; } evalContext = prevContext; - currentRollback = prevRollback; } if (valueIsError || this._valueIsError || this._value !== value) { @@ -393,22 +383,19 @@ class Effect { _start() { /*@__INLINE__**/ startBatch(); const prevContext = evalContext; - const prevRollback = currentRollback; evalContext = this; - currentRollback = undefined; subscribeDepth++; prepareSources(this); - return this._end.bind(this, prevContext, prevRollback); + return this._end.bind(this, prevContext); } - _end(prevContext?: Computed | Effect, prevRollback?: RollbackItem) { + _end(prevContext?: Computed | Effect) { cleanupSources(this); subscribeDepth--; evalContext = prevContext; - currentRollback = prevRollback; endBatch(); } From 76b4df3fd3b26f2349943a22c3822cc2ad25d125 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 09:28:44 +0300 Subject: [PATCH 26/66] Add explicit checks for undefined-ness This seems to boost (micro)benchmark results quite a bit. --- packages/core/src/index.ts | 48 +++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index d552c4dca..5cb88cb6a 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -36,13 +36,13 @@ function endBatch() { let error: unknown; let hasError = false; - while (batchedEffect) { + while (batchedEffect !== undefined) { let effect: Effect | undefined = batchedEffect; batchedEffect = undefined; batchIteration++; - while (effect) { + while (effect !== undefined) { const next: Effect | undefined = effect._nextEffect; effect._nextEffect = undefined; effect._batched = false; @@ -92,9 +92,9 @@ let globalVersion = 0; function getValue(signal: Signal): T { let node: Node | undefined = undefined; - if (evalContext) { + if (evalContext !== undefined) { node = signal._node; - if (!node || node.target !== evalContext) { + if (node === undefined || node.target !== evalContext) { node = { signal: signal, nextSignal: evalContext._sources, target: evalContext, version: 0, used: true, rollbackNode: node }; evalContext._sources = node; signal._node = node; @@ -111,7 +111,7 @@ function getValue(signal: Signal): T { try { return signal.peek(); } finally { - if (node) { + if (node !== undefined) { node.version = signal._version; } } @@ -136,7 +136,7 @@ export class Signal { /** @internal */ _subscribe(node: Node): void { - if (this._targets) { + if (this._targets !== undefined) { this._targets.prevTarget = node; } node.nextTarget = this._targets; @@ -150,10 +150,10 @@ export class Signal { const next = node.nextTarget; node.prevTarget = undefined; node.nextTarget = undefined; - if (prev) { + if (prev !== undefined) { prev.nextTarget = next; } - if (next) { + if (next !== undefined) { next.prevTarget = prev; } if (node === this._targets) { @@ -189,7 +189,7 @@ export class Signal { /**@__INLINE__*/ startBatch(); try { - for (let node = this._targets; node; node = node.nextTarget) { + for (let node = this._targets; node !== undefined; node = node.nextTarget) { node.target._invalidate(); } } finally { @@ -204,9 +204,9 @@ export function signal(value: T): Signal { } function prepareSources(target: Computed | Effect) { - for (let node = target._sources; node; node = node.nextSignal) { + for (let node = target._sources; node !== undefined; node = node.nextSignal) { const rollbackNode = node.signal._node; - if (rollbackNode) { + if (rollbackNode !== undefined) { node.rollbackNode = rollbackNode; } node.signal._node = node; @@ -217,7 +217,7 @@ function prepareSources(target: Computed | Effect) { function cleanupSources(target: Computed | Effect) { let sources = undefined; let node = target._sources; - while (node) { + while (node !== undefined) { const next = node.nextSignal; if (node.used) { node.nextSignal = sources; @@ -228,7 +228,7 @@ function cleanupSources(target: Computed | Effect) { } node.signal._node = node.rollbackNode; - if (node.rollbackNode) { + if (node.rollbackNode !== undefined) { node.rollbackNode = undefined; } node = next; @@ -259,11 +259,11 @@ export class Computed extends Signal { } _subscribe(node: Node) { - if (!this._targets) { + if (this._targets === undefined) { // A computed signal subscribes lazily to its dependencies when // the computed signal gets its first subscriber. this._valid = false; - for (let node = this._sources; node; node = node.nextSignal) { + for (let node = this._sources; node !== undefined; node = node.nextSignal) { node.signal._subscribe(node); } } @@ -273,8 +273,8 @@ export class Computed extends Signal { _unsubscribe(node: Node) { // When a computed signal loses its last subscriber it also unsubscribes // from its own dependencies. - if (!this._targets) { - for (let node = this._sources; node; node = node.nextSignal) { + if (this._targets === undefined) { + for (let node = this._sources; node !== undefined; node = node.nextSignal) { node.signal._unsubscribe(node); } } @@ -284,7 +284,7 @@ export class Computed extends Signal { _invalidate() { if (this._valid) { this._valid = false; - for (let node = this._targets; node; node = node.nextTarget) { + for (let node = this._targets; node !== undefined; node = node.nextTarget) { node.target._invalidate(); } } @@ -297,20 +297,20 @@ export class Computed extends Signal { if (this._globalVersion === globalVersion) { return returnComputed(this); } - if (this._targets && this._valid) { + if (this._valid && this._targets !== undefined) { return returnComputed(this); } if (this._version > 0) { let node = this._sources; - while (node) { + while (node !== undefined) { node.signal.peek(); if (node.signal._version !== node.version) { break; } node = node.nextSignal; } - if (!node) { + if (node === undefined) { return returnComputed(this); } } @@ -322,7 +322,7 @@ export class Computed extends Signal { const prevContext = evalContext; try { evalContext = this; - if (targets) { + if (targets !== undefined) { // Computed signals with current targets should automatically subscribe to // new dependencies it uses in the compute function. subscribeDepth++; @@ -339,7 +339,7 @@ export class Computed extends Signal { this._computing = false; cleanupSources(this); - if (targets) { + if (targets !== undefined) { subscribeDepth--; } evalContext = prevContext; @@ -408,7 +408,7 @@ class Effect { } _dispose() { - for (let node = this._sources; node; node = node.nextSignal) { + for (let node = this._sources; node !== undefined; node = node.nextSignal) { node.signal._unsubscribe(node); } this._sources = undefined; From 974dab747ec8314d296ed5c25dcb3b10f7f6d8c2 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 09:18:44 +0000 Subject: [PATCH 27/66] Fix computed error-handling related issues Add tests. --- packages/core/src/index.ts | 9 +++++++-- packages/core/test/signal.test.tsx | 29 +++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 5cb88cb6a..b559d652c 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -304,7 +304,12 @@ export class Computed extends Signal { if (this._version > 0) { let node = this._sources; while (node !== undefined) { - node.signal.peek(); + try { + node.signal.peek(); + } catch { + // Failures of previous dependencies shouldn't be rethrown here + // in case they're not dependencies anymore. + } if (node.signal._version !== node.version) { break; } @@ -345,7 +350,7 @@ export class Computed extends Signal { evalContext = prevContext; } - if (valueIsError || this._valueIsError || this._value !== value) { + if (valueIsError || this._valueIsError || this._value !== value || this._version === 0) { this._value = value; this._valueIsError = valueIsError; this._version++; diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index 6cba2a4fc..7872f18ad 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -210,6 +210,35 @@ 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"); + }); + describe("graph updates", () => { it("should run computeds once for multiple dep changes", async () => { const a = signal("a"); From 6e84a6177deeb82b70920241834eea66e16bab92 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 09:25:52 +0000 Subject: [PATCH 28/66] Add basic computed signal tests --- packages/core/test/signal.test.tsx | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index 7872f18ad..e3d27e262 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -182,6 +182,34 @@ 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 conditionally unsubscribe from signals", () => { const a = signal("a"); const b = signal("b"); From f9940981ccb2b8d50ecd267b42bc91938c504012 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 09:32:25 +0000 Subject: [PATCH 29/66] Add dependency cycle tests --- packages/core/src/index.ts | 2 +- packages/core/test/signal.test.tsx | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index b559d652c..de2406c3d 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -292,7 +292,7 @@ export class Computed extends Signal { peek(): T { if (this._computing) { - throw new Error("cycle detected"); + throw new Error("Cycle detected"); } if (this._globalVersion === globalVersion) { return returnComputed(this); diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index e3d27e262..a317012ea 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -210,6 +210,19 @@ describe("computed()", () => { 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 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 conditionally unsubscribe from signals", () => { const a = signal("a"); const b = signal("b"); From 02f8b40cd9baad422524c64cbec0a50006bcd3c7 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 10:04:47 +0000 Subject: [PATCH 30/66] Add tests --- packages/core/src/index.ts | 3 +++ packages/core/test/signal.test.tsx | 28 ++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index de2406c3d..bb66b9ad2 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -359,6 +359,9 @@ export class Computed extends Signal { } get value(): T { + if (this._computing) { + throw new Error("Cycle detected"); + } return getValue(this); } diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index a317012ea..08a0fae6c 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -215,6 +215,19 @@ describe("computed()", () => { expect(() => a.value).to.throw(/Cycle detected/); }); + 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); @@ -223,6 +236,21 @@ describe("computed()", () => { 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"); From a7ace11fb1e883b748707e42baa58e71d07b64fc Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Wed, 14 Sep 2022 14:08:32 +0000 Subject: [PATCH 31/66] Fix a invalidity propagation in specific cases --- packages/core/src/index.ts | 31 ++++++++++++++++++++---------- packages/core/test/signal.test.tsx | 18 +++++++++++++++++ 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index bb66b9ad2..5ef63058d 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -236,8 +236,14 @@ function cleanupSources(target: Computed | Effect) { target._sources = sources; } +const enum ComputedState { + Invalidated, + Revalidate, + Valid +} + function returnComputed(computed: Computed): T { - computed._valid = true; + computed._state = ComputedState.Valid; computed._globalVersion = globalVersion; if (computed._valueIsError) { throw computed._value; @@ -249,7 +255,7 @@ export class Computed extends Signal { _compute: () => T; _sources?: Node = undefined; _computing = false; - _valid = false; + _state = ComputedState.Revalidate; _valueIsError = false; _globalVersion = globalVersion - 1; @@ -260,9 +266,11 @@ export class Computed extends Signal { _subscribe(node: Node) { if (this._targets === undefined) { - // A computed signal subscribes lazily to its dependencies when - // the computed signal gets its first subscriber. - this._valid = false; + // The computed value may be up to date, or not. + this._state = ComputedState.Revalidate; + + // A computed signal subscribes lazily to its dependencies when the computed + // signal gets its first subscriber. for (let node = this._sources; node !== undefined; node = node.nextSignal) { node.signal._subscribe(node); } @@ -271,6 +279,8 @@ export class Computed extends Signal { } _unsubscribe(node: Node) { + super._unsubscribe(node) + // When a computed signal loses its last subscriber it also unsubscribes // from its own dependencies. if (this._targets === undefined) { @@ -278,12 +288,13 @@ export class Computed extends Signal { node.signal._unsubscribe(node); } } - super._unsubscribe(node) } _invalidate() { - if (this._valid) { - this._valid = false; + if (this._state !== ComputedState.Invalidated) { + // Mark the computed value as definitely out of date. + this._state = ComputedState.Invalidated; + for (let node = this._targets; node !== undefined; node = node.nextTarget) { node.target._invalidate(); } @@ -297,7 +308,7 @@ export class Computed extends Signal { if (this._globalVersion === globalVersion) { return returnComputed(this); } - if (this._valid && this._targets !== undefined) { + if (this._state === ComputedState.Valid && this._targets !== undefined) { return returnComputed(this); } @@ -337,7 +348,7 @@ export class Computed extends Signal { this._computing = true; value = this._compute(); - } catch (err: unknown) { + } catch (err) { valueIsError = true; value = err; } finally { diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index 08a0fae6c..27e972b3e 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -308,6 +308,24 @@ describe("computed()", () => { 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; + }); + describe("graph updates", () => { it("should run computeds once for multiple dep changes", async () => { const a = signal("a"); From 58ed8233d152eb0fc5fd0ed34f65668323330a42 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 06:54:57 +0000 Subject: [PATCH 32/66] Move Effect._end into an external function --- packages/core/src/index.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 5ef63058d..53660f116 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -389,6 +389,14 @@ export function computed(compute: () => T): ReadonlySignal { return new Computed(compute); } +function endEffect(this: Effect, prevContext?: Computed | Effect) { + cleanupSources(this); + + subscribeDepth--; + evalContext = prevContext; + endBatch(); +} + class Effect { _notify: () => void; _sources?: Node = undefined; @@ -407,15 +415,7 @@ class Effect { subscribeDepth++; prepareSources(this); - return this._end.bind(this, prevContext); - } - - _end(prevContext?: Computed | Effect) { - cleanupSources(this); - - subscribeDepth--; - evalContext = prevContext; - endBatch(); + return endEffect.bind(this, prevContext); } _invalidate() { From 0d58188e24febeffd127d75f699e30f2aa9077ef Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 10:49:53 +0000 Subject: [PATCH 33/66] Rename Effect._notify to ._callback --- packages/core/src/index.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 53660f116..b376844cb 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -47,7 +47,7 @@ function endBatch() { effect._nextEffect = undefined; effect._batched = false; try { - effect._notify(); + effect._callback(); } catch (err) { if (!hasError) { error = err; @@ -398,13 +398,13 @@ function endEffect(this: Effect, prevContext?: Computed | Effect) { } class Effect { - _notify: () => void; + _callback: () => void; _sources?: Node = undefined; _batched = false; _nextEffect?: Effect = undefined; - constructor(notify: () => void) { - this._notify = notify; + constructor(callback: () => void) { + this._callback = callback; } _start() { @@ -443,7 +443,7 @@ export function effect(callback: () => void): () => void { finish(); } }); - effect._notify(); + 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); From 1a27bfeee5eb6ce44f6eb3c4f003e46b0c816dce Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 10:52:05 +0000 Subject: [PATCH 34/66] Rename internal methods --- packages/core/src/index.ts | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index b376844cb..b3c741862 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -190,7 +190,7 @@ export class Signal { /**@__INLINE__*/ startBatch(); try { for (let node = this._targets; node !== undefined; node = node.nextTarget) { - node.target._invalidate(); + node.target._notify(); } } finally { endBatch(); @@ -237,13 +237,13 @@ function cleanupSources(target: Computed | Effect) { } const enum ComputedState { - Invalidated, - Revalidate, - Valid + Notified, + Stale, + Ok } function returnComputed(computed: Computed): T { - computed._state = ComputedState.Valid; + computed._state = ComputedState.Ok; computed._globalVersion = globalVersion; if (computed._valueIsError) { throw computed._value; @@ -255,7 +255,7 @@ export class Computed extends Signal { _compute: () => T; _sources?: Node = undefined; _computing = false; - _state = ComputedState.Revalidate; + _state = ComputedState.Stale; _valueIsError = false; _globalVersion = globalVersion - 1; @@ -267,7 +267,7 @@ export class Computed extends Signal { _subscribe(node: Node) { if (this._targets === undefined) { // The computed value may be up to date, or not. - this._state = ComputedState.Revalidate; + this._state = ComputedState.Stale; // A computed signal subscribes lazily to its dependencies when the computed // signal gets its first subscriber. @@ -290,13 +290,13 @@ export class Computed extends Signal { } } - _invalidate() { - if (this._state !== ComputedState.Invalidated) { + _notify() { + if (this._state !== ComputedState.Notified) { // Mark the computed value as definitely out of date. - this._state = ComputedState.Invalidated; + this._state = ComputedState.Notified; for (let node = this._targets; node !== undefined; node = node.nextTarget) { - node.target._invalidate(); + node.target._notify(); } } } @@ -308,7 +308,7 @@ export class Computed extends Signal { if (this._globalVersion === globalVersion) { return returnComputed(this); } - if (this._state === ComputedState.Valid && this._targets !== undefined) { + if (this._state === ComputedState.Ok && this._targets !== undefined) { return returnComputed(this); } @@ -418,7 +418,7 @@ class Effect { return endEffect.bind(this, prevContext); } - _invalidate() { + _notify() { if (!this._batched) { this._batched = true; this._nextEffect = batchedEffect; From e1238d69b372ff263383c4febe9cdf365b3f4b4b Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 11:06:11 +0000 Subject: [PATCH 35/66] Fix internal flags --- packages/core/src/index.ts | 50 ++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 27 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index b3c741862..73b7847c7 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -45,7 +45,7 @@ function endBatch() { while (effect !== undefined) { const next: Effect | undefined = effect._nextEffect; effect._nextEffect = undefined; - effect._batched = false; + effect._notified = false; try { effect._callback(); } catch (err) { @@ -236,15 +236,8 @@ function cleanupSources(target: Computed | Effect) { target._sources = sources; } -const enum ComputedState { - Notified, - Stale, - Ok -} - function returnComputed(computed: Computed): T { - computed._state = ComputedState.Ok; - computed._globalVersion = globalVersion; + computed._running = false; if (computed._valueIsError) { throw computed._value; } @@ -254,8 +247,9 @@ function returnComputed(computed: Computed): T { export class Computed extends Signal { _compute: () => T; _sources?: Node = undefined; - _computing = false; - _state = ComputedState.Stale; + _running = false; + _stale = true; + _notified = false; _valueIsError = false; _globalVersion = globalVersion - 1; @@ -266,8 +260,7 @@ export class Computed extends Signal { _subscribe(node: Node) { if (this._targets === undefined) { - // The computed value may be up to date, or not. - this._state = ComputedState.Stale; + this._stale = true; // A computed signal subscribes lazily to its dependencies when the computed // signal gets its first subscriber. @@ -291,9 +284,9 @@ export class Computed extends Signal { } _notify() { - if (this._state !== ComputedState.Notified) { - // Mark the computed value as definitely out of date. - this._state = ComputedState.Notified; + if (!this._notified) { + this._notified = true; + this._stale = true; for (let node = this._targets; node !== undefined; node = node.nextTarget) { node.target._notify(); @@ -302,15 +295,22 @@ export class Computed extends Signal { } peek(): T { - if (this._computing) { + this._notified = false; + + if (this._running) { throw new Error("Cycle detected"); } - if (this._globalVersion === globalVersion) { + this._running = true; + + if (!this._stale && this._targets !== undefined) { return returnComputed(this); } - if (this._state === ComputedState.Ok && this._targets !== undefined) { + this._stale = false; + + if (this._globalVersion === globalVersion) { return returnComputed(this); } + this._globalVersion = globalVersion; if (this._version > 0) { let node = this._sources; @@ -343,17 +343,13 @@ export class Computed extends Signal { // new dependencies it uses in the compute function. subscribeDepth++; } - prepareSources(this); - this._computing = true; value = this._compute(); } catch (err) { valueIsError = true; value = err; } finally { - this._computing = false; - cleanupSources(this); if (targets !== undefined) { subscribeDepth--; @@ -370,7 +366,7 @@ export class Computed extends Signal { } get value(): T { - if (this._computing) { + if (this._running) { throw new Error("Cycle detected"); } return getValue(this); @@ -400,7 +396,7 @@ function endEffect(this: Effect, prevContext?: Computed | Effect) { class Effect { _callback: () => void; _sources?: Node = undefined; - _batched = false; + _notified = false; _nextEffect?: Effect = undefined; constructor(callback: () => void) { @@ -419,8 +415,8 @@ class Effect { } _notify() { - if (!this._batched) { - this._batched = true; + if (!this._notified) { + this._notified = true; this._nextEffect = batchedEffect; batchedEffect = this; } From 22cb567f37c56a3907da2e7cd0c7ce9766ec97b8 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 11:08:57 +0000 Subject: [PATCH 36/66] Add _running flag to Effects --- packages/core/src/index.ts | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 73b7847c7..8dd27aad8 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -1,3 +1,7 @@ +function cycleDetected() { + throw new Error("Cycle detected"); +} + // 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 = { @@ -180,7 +184,7 @@ export class Signal { set value(value: T) { if (value !== this._value) { if (batchIteration > 100) { - throw new Error("Cycle detected"); + cycleDetected(); } this._value = value; @@ -298,7 +302,7 @@ export class Computed extends Signal { this._notified = false; if (this._running) { - throw new Error("Cycle detected"); + cycleDetected(); } this._running = true; @@ -367,7 +371,7 @@ export class Computed extends Signal { get value(): T { if (this._running) { - throw new Error("Cycle detected"); + cycleDetected(); } return getValue(this); } @@ -391,11 +395,14 @@ function endEffect(this: Effect, prevContext?: Computed | Effect) { subscribeDepth--; evalContext = prevContext; endBatch(); + + this._running = false; } class Effect { _callback: () => void; _sources?: Node = undefined; + _running = false; _notified = false; _nextEffect?: Effect = undefined; @@ -404,6 +411,11 @@ class Effect { } _start() { + if (this._running) { + cycleDetected(); + } + this._running = true; + /*@__INLINE__**/ startBatch(); const prevContext = evalContext; From 1609b2d56653553d6a57905f9fbd210cadbe33ef Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 11:21:25 +0000 Subject: [PATCH 37/66] Use bit flags internally in Computed --- packages/core/src/index.ts | 41 +++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 8dd27aad8..584ed0ff7 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -2,6 +2,11 @@ function cycleDetected() { throw new Error("Cycle detected"); } +const RUNNING = 1 << 0; +const STALE = 1 << 1; +const NOTIFIED = 1 << 2; +const HAS_ERROR = 1 << 3; + // 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 = { @@ -241,8 +246,8 @@ function cleanupSources(target: Computed | Effect) { } function returnComputed(computed: Computed): T { - computed._running = false; - if (computed._valueIsError) { + computed._flags &= ~RUNNING; + if (computed._flags & HAS_ERROR) { throw computed._value; } return computed._value as T; @@ -251,11 +256,8 @@ function returnComputed(computed: Computed): T { export class Computed extends Signal { _compute: () => T; _sources?: Node = undefined; - _running = false; - _stale = true; - _notified = false; - _valueIsError = false; _globalVersion = globalVersion - 1; + _flags = STALE; constructor(compute: () => T) { super(undefined); @@ -264,7 +266,7 @@ export class Computed extends Signal { _subscribe(node: Node) { if (this._targets === undefined) { - this._stale = true; + this._flags |= STALE; // A computed signal subscribes lazily to its dependencies when the computed // signal gets its first subscriber. @@ -288,9 +290,8 @@ export class Computed extends Signal { } _notify() { - if (!this._notified) { - this._notified = true; - this._stale = true; + if (!(this._flags & NOTIFIED)) { + this._flags |= STALE | NOTIFIED; for (let node = this._targets; node !== undefined; node = node.nextTarget) { node.target._notify(); @@ -299,17 +300,17 @@ export class Computed extends Signal { } peek(): T { - this._notified = false; + this._flags &= ~NOTIFIED; - if (this._running) { + if (this._flags & RUNNING) { cycleDetected(); } - this._running = true; + this._flags |= RUNNING; - if (!this._stale && this._targets !== undefined) { + if (!(this._flags & STALE) && this._targets !== undefined) { return returnComputed(this); } - this._stale = false; + this._flags &= ~STALE; if (this._globalVersion === globalVersion) { return returnComputed(this); @@ -361,16 +362,20 @@ export class Computed extends Signal { evalContext = prevContext; } - if (valueIsError || this._valueIsError || this._value !== value || this._version === 0) { + if (valueIsError || (this._flags & HAS_ERROR) || this._value !== value || this._version === 0) { + if (valueIsError) { + this._flags |= HAS_ERROR; + } else { + this._flags &= ~HAS_ERROR; + } this._value = value; - this._valueIsError = valueIsError; this._version++; } return returnComputed(this); } get value(): T { - if (this._running) { + if (this._flags & RUNNING) { cycleDetected(); } return getValue(this); From 088b5843c6a634f9520bcee6aa61969ff2e7bf02 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 11:24:38 +0000 Subject: [PATCH 38/66] Use bit flags internally in Effect --- packages/core/src/index.ts | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 584ed0ff7..5b13ca601 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -54,7 +54,7 @@ function endBatch() { while (effect !== undefined) { const next: Effect | undefined = effect._nextEffect; effect._nextEffect = undefined; - effect._notified = false; + effect._flags &= ~NOTIFIED; try { effect._callback(); } catch (err) { @@ -401,25 +401,24 @@ function endEffect(this: Effect, prevContext?: Computed | Effect) { evalContext = prevContext; endBatch(); - this._running = false; + this._flags &= ~RUNNING; } class Effect { _callback: () => void; _sources?: Node = undefined; - _running = false; - _notified = false; _nextEffect?: Effect = undefined; + _flags = 0; constructor(callback: () => void) { this._callback = callback; } _start() { - if (this._running) { + if (this._flags & RUNNING) { cycleDetected(); } - this._running = true; + this._flags |= RUNNING; /*@__INLINE__**/ startBatch(); const prevContext = evalContext; @@ -432,8 +431,8 @@ class Effect { } _notify() { - if (!this._notified) { - this._notified = true; + if (!(this._flags & NOTIFIED)) { + this._flags |= NOTIFIED; this._nextEffect = batchedEffect; batchedEffect = this; } From 3cff2725f73d3661e7c879071720b25172f7143c Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 15:55:49 +0000 Subject: [PATCH 39/66] Keep dependencies in order --- packages/core/src/index.ts | 60 ++++++++++++++++++++++++++++-- packages/core/test/signal.test.tsx | 35 +++++++++++++++++ 2 files changed, 91 insertions(+), 4 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 5b13ca601..2a4710558 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -12,6 +12,7 @@ const HAS_ERROR = 1 << 3; type Node = { // A source whose value the target depends on. signal: Signal; + prevSignal?: Node; nextSignal?: Node; // A target that depends on the source and should be notified when the source changes. @@ -104,16 +105,51 @@ function getValue(signal: Signal): T { if (evalContext !== undefined) { node = signal._node; if (node === undefined || node.target !== evalContext) { - node = { signal: signal, nextSignal: evalContext._sources, target: evalContext, version: 0, used: true, rollbackNode: node }; + // `signal` is a new dependency. Create a new node dependency node, move it + // to the front of the current context's dependency list. + node = { + signal: signal, + nextSignal: evalContext._sources, + target: evalContext, + version: 0, + used: true, + 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 (subscribeDepth > 0) { signal._subscribe(node); } } else if (!node.used) { + // `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.used = true; + + const head = evalContext._sources; + if (node !== head) { + const prev = node.prevSignal; + const next = node.nextSignal; + if (prev) { + prev.nextSignal = next; + } + if (next) { + next.prevSignal = prev; + } + if (head !== undefined) { + head.prevSignal = node; + } + node.prevSignal = undefined; + node.nextSignal = head; + evalContext._sources = node; + } + + // 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; } } @@ -224,11 +260,21 @@ function prepareSources(target: Computed | Effect) { } function cleanupSources(target: Computed | Effect) { - let sources = undefined; + // 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.nextSignal; if (node.used) { + if (sources) { + sources.prevSignal = node; + } + node.prevSignal = undefined; node.nextSignal = sources; sources = node; } else { @@ -318,13 +364,19 @@ export class Computed extends Signal { 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.signal._version !== node.version) { + break; + } try { node.signal.peek(); } catch { - // Failures of previous dependencies shouldn't be rethrown here - // in case they're not dependencies anymore. + // Failures of current dependencies shouldn't be rethrown here in case the + // compute function catches them. } if (node.signal._version !== node.version) { break; diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index 27e972b3e..192bf7e9a 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -326,6 +326,41 @@ describe("computed()", () => { 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"); From ff860405e81e90a82c4ab14dfaa30c850fb1cb12 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 16:03:57 +0000 Subject: [PATCH 40/66] Remove subscribeDepth from global state --- packages/core/src/index.ts | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 2a4710558..244544347 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -6,6 +6,7 @@ const RUNNING = 1 << 0; const STALE = 1 << 1; const NOTIFIED = 1 << 2; const HAS_ERROR = 1 << 3; +const SHOULD_SUBSCRIBE = 1 << 4; // 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. @@ -89,13 +90,12 @@ export function batch(callback: () => T): T { // Currently evaluated computed or effect. let evalContext: Computed | Effect | undefined = undefined; -// Used for keeping track whether the current evaluation context should automatically -// subscribe for updates to the signals depends on. -let subscribeDepth = 0; + // Effects collected into a batch. let batchedEffect: Effect | undefined = undefined; let batchDepth = 0; let batchIteration = 0; + // A global version number for signalss, used for fast-pathing repeated // computed.peek()/computed.value calls when nothing has changed globally. let globalVersion = 0; @@ -120,7 +120,7 @@ function getValue(signal: Signal): T { // Subscribe to change notifications from this dependency if we're in an effect // OR evaluating a computed signal that in turn has subscribers. - if (subscribeDepth > 0) { + if (evalContext._flags & SHOULD_SUBSCRIBE) { signal._subscribe(node); } } else if (!node.used) { @@ -312,7 +312,7 @@ export class Computed extends Signal { _subscribe(node: Node) { if (this._targets === undefined) { - this._flags |= STALE; + this._flags |= STALE | SHOULD_SUBSCRIBE; // A computed signal subscribes lazily to its dependencies when the computed // signal gets its first subscriber. @@ -329,6 +329,8 @@ export class Computed extends Signal { // When a computed signal loses its last subscriber it also unsubscribes // from its own dependencies. if (this._targets === undefined) { + this._flags &= ~SHOULD_SUBSCRIBE; + for (let node = this._sources; node !== undefined; node = node.nextSignal) { node.signal._unsubscribe(node); } @@ -391,26 +393,16 @@ export class Computed extends Signal { let value: unknown = undefined; let valueIsError = false; - const targets = this._targets; const prevContext = evalContext; try { evalContext = this; - if (targets !== undefined) { - // Computed signals with current targets should automatically subscribe to - // new dependencies it uses in the compute function. - subscribeDepth++; - } prepareSources(this); - value = this._compute(); } catch (err) { valueIsError = true; value = err; } finally { - cleanupSources(this); - if (targets !== undefined) { - subscribeDepth--; - } + cleanupSources(this) evalContext = prevContext; } @@ -449,7 +441,6 @@ export function computed(compute: () => T): ReadonlySignal { function endEffect(this: Effect, prevContext?: Computed | Effect) { cleanupSources(this); - subscribeDepth--; evalContext = prevContext; endBatch(); @@ -460,7 +451,7 @@ class Effect { _callback: () => void; _sources?: Node = undefined; _nextEffect?: Effect = undefined; - _flags = 0; + _flags = SHOULD_SUBSCRIBE; constructor(callback: () => void) { this._callback = callback; @@ -476,7 +467,6 @@ class Effect { const prevContext = evalContext; evalContext = this; - subscribeDepth++; prepareSources(this); return endEffect.bind(this, prevContext); From 2e437a4e833c20cf73b611b28d1c785246314246 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 16:46:07 +0000 Subject: [PATCH 41/66] Use bit flags for Nodes --- packages/core/src/index.ts | 162 ++++++++++++++++++++----------------- 1 file changed, 86 insertions(+), 76 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 244544347..9f1518f16 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -7,31 +7,34 @@ const STALE = 1 << 1; const NOTIFIED = 1 << 2; const HAS_ERROR = 1 << 3; const SHOULD_SUBSCRIBE = 1 << 4; +const SUBSCRIBED = 1 << 5; // 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 `.target` has subscribed to listen change notifications from `.signal` + // STALE when it's unclear whether `.signal` is still a dependency of `.target` + _flags: number; + // A source whose value the target depends on. - signal: Signal; - prevSignal?: Node; - nextSignal?: Node; + _signal: Signal; + _prevSignal?: Node; + _nextSignal?: Node; // A target that depends on the source and should be notified when the source changes. - target: Computed | Effect; - prevTarget?: Node; - nextTarget?: Node; + _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; - - // Whether the target is currently depending the signal. - used: boolean; + _version: number; // Used to remember & roll back signal's previous `._node` value when entering & exiting // a new evaluation context. - rollbackNode?: Node; + _rollbackNode?: Node; }; function startBatch() { @@ -104,16 +107,16 @@ function getValue(signal: Signal): T { let node: Node | undefined = undefined; if (evalContext !== undefined) { node = signal._node; - if (node === undefined || node.target !== evalContext) { + 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 = { - signal: signal, - nextSignal: evalContext._sources, - target: evalContext, - version: 0, - used: true, - rollbackNode: node + _flags: 0, + _version: 0, + _signal: signal, + _nextSignal: evalContext._sources, + _target: evalContext, + _rollbackNode: node }; evalContext._sources = node; signal._node = node; @@ -123,26 +126,26 @@ function getValue(signal: Signal): T { if (evalContext._flags & SHOULD_SUBSCRIBE) { signal._subscribe(node); } - } else if (!node.used) { + } 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.used = true; + node._flags &= ~STALE; const head = evalContext._sources; if (node !== head) { - const prev = node.prevSignal; - const next = node.nextSignal; + const prev = node._prevSignal; + const next = node._nextSignal; if (prev) { - prev.nextSignal = next; + prev._nextSignal = next; } if (next) { - next.prevSignal = prev; + next._prevSignal = prev; } if (head !== undefined) { - head.prevSignal = node; + head._prevSignal = node; } - node.prevSignal = undefined; - node.nextSignal = head; + node._prevSignal = undefined; + node._nextSignal = head; evalContext._sources = node; } @@ -157,7 +160,7 @@ function getValue(signal: Signal): T { return signal.peek(); } finally { if (node !== undefined) { - node.version = signal._version; + node._version = signal._version; } } } @@ -181,28 +184,35 @@ export class Signal { /** @internal */ _subscribe(node: Node): void { - if (this._targets !== undefined) { - this._targets.prevTarget = node; + if (!(node._flags & SUBSCRIBED)) { + node._flags |= SUBSCRIBED; + node._nextTarget = this._targets; + + if (this._targets !== undefined) { + this._targets._prevTarget = node; + } + this._targets = node; } - node.nextTarget = this._targets; - node.prevTarget = undefined; - this._targets = node; } /** @internal */ _unsubscribe(node: Node): void { - const prev = node.prevTarget; - const next = node.nextTarget; - node.prevTarget = undefined; - node.nextTarget = undefined; - if (prev !== undefined) { - prev.nextTarget = next; - } - if (next !== undefined) { - next.prevTarget = prev; - } - if (node === this._targets) { - this._targets = next; + if (node._flags & SUBSCRIBED) { + node._flags &= ~SUBSCRIBED; + + 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; + } } } @@ -234,8 +244,8 @@ export class Signal { /**@__INLINE__*/ startBatch(); try { - for (let node = this._targets; node !== undefined; node = node.nextTarget) { - node.target._notify(); + for (let node = this._targets; node !== undefined; node = node._nextTarget) { + node._target._notify(); } } finally { endBatch(); @@ -249,13 +259,13 @@ export function signal(value: T): Signal { } function prepareSources(target: Computed | Effect) { - for (let node = target._sources; node !== undefined; node = node.nextSignal) { - const rollbackNode = node.signal._node; + for (let node = target._sources; node !== undefined; node = node._nextSignal) { + const rollbackNode = node._signal._node; if (rollbackNode !== undefined) { - node.rollbackNode = rollbackNode; + node._rollbackNode = rollbackNode; } - node.signal._node = node; - node.used = false; + node._signal._node = node; + node._flags |= STALE; } } @@ -269,22 +279,22 @@ function cleanupSources(target: Computed | Effect) { let node = target._sources; let sources = undefined; while (node !== undefined) { - const next = node.nextSignal; - if (node.used) { - if (sources) { - sources.prevSignal = node; + const next = node._nextSignal; + if (node._flags & STALE) { + node._signal._unsubscribe(node); + node._nextSignal = undefined; + } else { + if (sources !== undefined) { + sources._prevSignal = node; } - node.prevSignal = undefined; - node.nextSignal = sources; + node._prevSignal = undefined; + node._nextSignal = sources; sources = node; - } else { - node.signal._unsubscribe(node); - node.nextSignal = undefined; } - node.signal._node = node.rollbackNode; - if (node.rollbackNode !== undefined) { - node.rollbackNode = undefined; + node._signal._node = node._rollbackNode; + if (node._rollbackNode !== undefined) { + node._rollbackNode = undefined; } node = next; } @@ -316,8 +326,8 @@ export class Computed extends Signal { // A computed signal subscribes lazily to its dependencies when the computed // signal gets its first subscriber. - for (let node = this._sources; node !== undefined; node = node.nextSignal) { - node.signal._subscribe(node); + for (let node = this._sources; node !== undefined; node = node._nextSignal) { + node._signal._subscribe(node); } } super._subscribe(node); @@ -331,8 +341,8 @@ export class Computed extends Signal { if (this._targets === undefined) { this._flags &= ~SHOULD_SUBSCRIBE; - for (let node = this._sources; node !== undefined; node = node.nextSignal) { - node.signal._unsubscribe(node); + for (let node = this._sources; node !== undefined; node = node._nextSignal) { + node._signal._unsubscribe(node); } } } @@ -341,8 +351,8 @@ export class Computed extends Signal { if (!(this._flags & NOTIFIED)) { this._flags |= STALE | NOTIFIED; - for (let node = this._targets; node !== undefined; node = node.nextTarget) { - node.target._notify(); + for (let node = this._targets; node !== undefined; node = node._nextTarget) { + node._target._notify(); } } } @@ -371,19 +381,19 @@ export class Computed extends Signal { // is re-evaluated at this point. let node = this._sources; while (node !== undefined) { - if (node.signal._version !== node.version) { + if (node._signal._version !== node._version) { break; } try { - node.signal.peek(); + node._signal.peek(); } catch { // Failures of current dependencies shouldn't be rethrown here in case the // compute function catches them. } - if (node.signal._version !== node.version) { + if (node._signal._version !== node._version) { break; } - node = node.nextSignal; + node = node._nextSignal; } if (node === undefined) { return returnComputed(this); @@ -481,8 +491,8 @@ class Effect { } _dispose() { - for (let node = this._sources; node !== undefined; node = node.nextSignal) { - node.signal._unsubscribe(node); + for (let node = this._sources; node !== undefined; node = node._nextSignal) { + node._signal._unsubscribe(node); } this._sources = undefined; } From 915fa02962a379ea4f4e4e8ad817e9dd46a42a10 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 17:10:24 +0000 Subject: [PATCH 42/66] Refactor --- packages/core/src/index.ts | 81 +++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 41 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 9f1518f16..73c184fb2 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -13,14 +13,14 @@ const SUBSCRIBED = 1 << 5; // 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 `.target` has subscribed to listen change notifications from `.signal` - // STALE when it's unclear whether `.signal` is still a dependency of `.target` + // 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. - _signal: Signal; - _prevSignal?: Node; - _nextSignal?: Node; + _source: Signal; + _prevSource?: Node; + _nextSource?: Node; // A target that depends on the source and should be notified when the source changes. _target: Computed | Effect; @@ -32,8 +32,8 @@ type Node = { // of memory, and computeds could hang on to them forever because they're lazily evaluated. _version: number; - // Used to remember & roll back signal's previous `._node` value when entering & exiting - // a new evaluation context. + // Used to remember & roll back the source's previous `._node` value when entering & + // exiting a new evaluation context. _rollbackNode?: Node; }; @@ -99,7 +99,7 @@ let batchedEffect: Effect | undefined = undefined; let batchDepth = 0; let batchIteration = 0; -// A global version number for signalss, used for fast-pathing repeated +// A global version number for signals, used for fast-pathing repeated // computed.peek()/computed.value calls when nothing has changed globally. let globalVersion = 0; @@ -113,8 +113,8 @@ function getValue(signal: Signal): T { node = { _flags: 0, _version: 0, - _signal: signal, - _nextSignal: evalContext._sources, + _source: signal, + _nextSource: evalContext._sources, _target: evalContext, _rollbackNode: node }; @@ -133,19 +133,19 @@ function getValue(signal: Signal): T { const head = evalContext._sources; if (node !== head) { - const prev = node._prevSignal; - const next = node._nextSignal; + const prev = node._prevSource; + const next = node._nextSource; if (prev) { - prev._nextSignal = next; + prev._nextSource = next; } if (next) { - next._prevSignal = prev; + next._prevSource = prev; } if (head !== undefined) { - head._prevSignal = node; + head._prevSource = node; } - node._prevSignal = undefined; - node._nextSignal = head; + node._prevSource = undefined; + node._nextSource = head; evalContext._sources = node; } @@ -259,12 +259,12 @@ export function signal(value: T): Signal { } function prepareSources(target: Computed | Effect) { - for (let node = target._sources; node !== undefined; node = node._nextSignal) { - const rollbackNode = node._signal._node; + for (let node = target._sources; node !== undefined; node = node._nextSource) { + const rollbackNode = node._source._node; if (rollbackNode !== undefined) { node._rollbackNode = rollbackNode; } - node._signal._node = node; + node._source._node = node; node._flags |= STALE; } } @@ -279,20 +279,20 @@ function cleanupSources(target: Computed | Effect) { let node = target._sources; let sources = undefined; while (node !== undefined) { - const next = node._nextSignal; + const next = node._nextSource; if (node._flags & STALE) { - node._signal._unsubscribe(node); - node._nextSignal = undefined; + node._source._unsubscribe(node); + node._nextSource = undefined; } else { if (sources !== undefined) { - sources._prevSignal = node; + sources._prevSource = node; } - node._prevSignal = undefined; - node._nextSignal = sources; + node._prevSource = undefined; + node._nextSource = sources; sources = node; } - node._signal._node = node._rollbackNode; + node._source._node = node._rollbackNode; if (node._rollbackNode !== undefined) { node._rollbackNode = undefined; } @@ -324,10 +324,10 @@ export class Computed extends Signal { if (this._targets === undefined) { this._flags |= STALE | SHOULD_SUBSCRIBE; - // A computed signal subscribes lazily to its dependencies when the computed - // signal gets its first subscriber. - for (let node = this._sources; node !== undefined; node = node._nextSignal) { - node._signal._subscribe(node); + // 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); @@ -336,13 +336,12 @@ export class Computed extends Signal { _unsubscribe(node: Node) { super._unsubscribe(node) - // When a computed signal loses its last subscriber it also unsubscribes - // from its own dependencies. + // 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._nextSignal) { - node._signal._unsubscribe(node); + for (let node = this._sources; node !== undefined; node = node._nextSource) { + node._source._unsubscribe(node); } } } @@ -381,19 +380,19 @@ export class Computed extends Signal { // is re-evaluated at this point. let node = this._sources; while (node !== undefined) { - if (node._signal._version !== node._version) { + if (node._source._version !== node._version) { break; } try { - node._signal.peek(); + node._source.peek(); } catch { // Failures of current dependencies shouldn't be rethrown here in case the // compute function catches them. } - if (node._signal._version !== node._version) { + if (node._source._version !== node._version) { break; } - node = node._nextSignal; + node = node._nextSource; } if (node === undefined) { return returnComputed(this); @@ -491,8 +490,8 @@ class Effect { } _dispose() { - for (let node = this._sources; node !== undefined; node = node._nextSignal) { - node._signal._unsubscribe(node); + for (let node = this._sources; node !== undefined; node = node._nextSource) { + node._source._unsubscribe(node); } this._sources = undefined; } From 8c08ead0116ace372911e79076483fffacf02cfb Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 17:53:36 +0000 Subject: [PATCH 43/66] Save bytes by not implementing Computed.value setter --- packages/core/src/index.ts | 13 +++---------- packages/core/test/signal.test.tsx | 2 +- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 73c184fb2..ddaa51e4d 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -1,4 +1,4 @@ -function cycleDetected() { +function cycleDetected(): never { throw new Error("Cycle detected"); } @@ -433,19 +433,12 @@ export class Computed extends Signal { } return getValue(this); } - - set value(value: T) { - throw Error("Computed signals are readonly"); - } -} - -export interface ReadonlySignal extends Signal { - readonly value: T; } -export function computed(compute: () => T): ReadonlySignal { +export function computed(compute: () => T): Computed { return new Computed(compute); } +export type { Computed as ReadonlySignal }; function endEffect(this: Effect, prevContext?: Computed | Effect) { cleanupSources(this); diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index 192bf7e9a..e9bfa6012 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -659,7 +659,7 @@ describe("computed()", () => { const a = signal("a"); const b = computed(() => a.value); const fn = () => ((b as Signal).value = "aa"); - expect(fn).to.throw(/readonly/); + expect(fn).to.throw(/Cannot set property value/); }); it("should keep graph consistent on errors during activation", () => { From 3522c82824e93563c0ed1c7b32280ba0b84c0df8 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 18:01:34 +0000 Subject: [PATCH 44/66] Add a couple of explicit undefinedness checks --- packages/core/src/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index ddaa51e4d..c398f8a19 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -135,10 +135,10 @@ function getValue(signal: Signal): T { if (node !== head) { const prev = node._prevSource; const next = node._nextSource; - if (prev) { + if (prev !== undefined) { prev._nextSource = next; } - if (next) { + if (next !== undefined) { next._prevSource = prev; } if (head !== undefined) { From 2e7ed9a7f943d6cf6dc8df18699a1a7437f6b4a5 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 18:20:48 +0000 Subject: [PATCH 45/66] Minor code cleanup --- packages/core/src/index.ts | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index c398f8a19..d5dc1cc88 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -399,31 +399,25 @@ export class Computed extends Signal { } } - let value: unknown = undefined; - let valueIsError = false; - + const prevValue = this._value; + const prevFlags = this._flags; const prevContext = evalContext; try { evalContext = this; prepareSources(this); - value = this._compute(); + this._value = this._compute(); + this._flags &= ~HAS_ERROR; + if ((prevFlags & HAS_ERROR) || this._value !== prevValue || this._version === 0) { + this._version++; + } } catch (err) { - valueIsError = true; - value = err; + this._value = err; + this._flags |= HAS_ERROR; + this._version++; } finally { cleanupSources(this) evalContext = prevContext; } - - if (valueIsError || (this._flags & HAS_ERROR) || this._value !== value || this._version === 0) { - if (valueIsError) { - this._flags |= HAS_ERROR; - } else { - this._flags &= ~HAS_ERROR; - } - this._value = value; - this._version++; - } return returnComputed(this); } From af202ee3c3b60c65f0b10ae05431dae73b074c9f Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 21:02:42 +0000 Subject: [PATCH 46/66] Fix, don't export Computed --- packages/core/src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index d5dc1cc88..3e4cf1eea 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -309,7 +309,7 @@ function returnComputed(computed: Computed): T { return computed._value as T; } -export class Computed extends Signal { +class Computed extends Signal { _compute: () => T; _sources?: Node = undefined; _globalVersion = globalVersion - 1; From 9e581deb853bc9167bbc8c9cf76eb4740a48797c Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Thu, 15 Sep 2022 22:28:33 +0000 Subject: [PATCH 47/66] Optimize effect creation Avoid a closure on every effect() call. This improves the memory consumption of trivial effects by about 1/3 while being 4x faster (Node.js 18). --- packages/core/src/index.ts | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 3e4cf1eea..0a09bf7d2 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -444,13 +444,22 @@ function endEffect(this: Effect, prevContext?: Computed | Effect) { } class Effect { - _callback: () => void; + _compute: () => void; _sources?: Node = undefined; _nextEffect?: Effect = undefined; _flags = SHOULD_SUBSCRIBE; - constructor(callback: () => void) { - this._callback = callback; + constructor(compute: () => void) { + this._compute = compute; + } + + _callback() { + const finish = this._start(); + try { + this._compute(); + } finally { + finish(); + } } _start() { @@ -484,15 +493,8 @@ class Effect { } } -export function effect(callback: () => void): () => void { - const effect = new Effect(() => { - const finish = effect._start(); - try { - callback.call(effect); - } finally { - finish(); - } - }); +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. From 9d837e80daaa7a43a30e41e4fa370330cda08199 Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Thu, 15 Sep 2022 19:06:14 -0400 Subject: [PATCH 48/66] fix misnamed variable in #136 --- packages/core/test/signal.test.tsx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index e9bfa6012..2ce2f6bc4 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -218,11 +218,11 @@ describe("computed()", () => { it("should not allow a computed signal to become a direct dependency of itself", () => { const spy = sinon.spy(() => { try { - a.value + a.value; } catch { // pass } - }) + }); const a = computed(spy); a.value; expect(() => effect(() => a.value)).to.not.throw(); @@ -344,21 +344,21 @@ describe("computed()", () => { }); e.value; - spy.resetHistory() + spy.resetHistory(); a.value = 2; b.value = 2; c.value = 2; e.value; expect(spy).to.be.calledOnce; - spy.resetHistory() + spy.resetHistory(); a.value = -1; b.value = -1; c.value = -1; e.value; expect(spy).not.to.be.called; - spy.resetHistory() + spy.resetHistory(); }); describe("graph updates", () => { @@ -404,7 +404,7 @@ describe("computed()", () => { compute.resetHistory(); a.value = 4; - e.value; + d.value; expect(compute).to.have.been.calledOnce; }); From dd8320419b4bf12ac58044cf39d0d33cb1e33e76 Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Thu, 15 Sep 2022 19:06:26 -0400 Subject: [PATCH 49/66] Update Preact integration for #136 --- packages/preact/src/index.ts | 123 ++++++++++++++++-------------- packages/preact/src/internal.d.ts | 21 +++-- 2 files changed, 80 insertions(+), 64 deletions(-) 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", From e42199ed4f00cc751cdf0f0b9de44c818a28c044 Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Thu, 15 Sep 2022 19:06:31 -0400 Subject: [PATCH 50/66] Update React integration for #136 --- packages/react/src/index.ts | 25 ++++++++++++++----------- packages/react/src/internal.d.ts | 7 +++++++ 2 files changed, 21 insertions(+), 11 deletions(-) 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; } From 7bdfbc1f8f6d37878205df9024f427579c758579 Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Thu, 15 Sep 2022 19:06:40 -0400 Subject: [PATCH 51/66] Add all missing properties to mangle.json --- mangle.json | 41 ++++++++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/mangle.json b/mangle.json index 3a9e5df39..ddf087d94 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": "v", + "$_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": "", + "$_": "" } } } From 38139e8a1133cf546c0a173bdee0956607a562fb Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Thu, 15 Sep 2022 19:12:04 -0400 Subject: [PATCH 52/66] avoid collision between Effect._callback and Effect._compute --- mangle.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mangle.json b/mangle.json index ddf087d94..e5279969c 100644 --- a/mangle.json +++ b/mangle.json @@ -40,7 +40,7 @@ "$_compute": "v", "$_globalVersion": "g", "core: Effect": "", - "$_callback": "v", + "$_callback": "c", "$_nextEffect": "e", "$_start": "S", "$_dispose": "d", From 8ea2aa971811a25d3da00ad75b6ce7de102c3662 Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Thu, 15 Sep 2022 19:12:32 -0400 Subject: [PATCH 53/66] Add missing _updaters prop to mangle --- mangle.json | 112 ++++++++++++++++++++++++++-------------------------- 1 file changed, 56 insertions(+), 56 deletions(-) diff --git a/mangle.json b/mangle.json index e5279969c..9dd9f456d 100644 --- a/mangle.json +++ b/mangle.json @@ -1,59 +1,59 @@ { - "help": { - "what is this file?": "It controls protected/private property mangling so that minified builds have consistent property names.", - "why are there duplicate minified properties?": "Most properties are only used on one type of objects, so they can have the same name since they will never collide. Doing this reduces size." - }, - "minify": { - "mangle": { - "keep_classnames": true, - "properties": { - "regex": "^_[^_]", - "reserved": [ - "__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED", - "__REACT_DEVTOOLS_GLOBAL_HOOK__", - "__PREACT_DEVTOOLS__", - "_renderers", - "_" - ] - } - }, - "compress": { - "reduce_funcs": false - } - }, - "props": { - "cname": 6, - "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": "", + "help": { + "what is this file?": "It controls protected/private property mangling so that minified builds have consistent property names.", + "why are there duplicate minified properties?": "Most properties are only used on one type of objects, so they can have the same name since they will never collide. Doing this reduces size." + }, + "minify": { + "mangle": { + "keep_classnames": true, + "properties": { + "regex": "^_[^_]", + "reserved": [ + "__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED", + "__REACT_DEVTOOLS_GLOBAL_HOOK__", + "__PREACT_DEVTOOLS__", + "_renderers", + "_" + ] + } + }, + "compress": { + "reduce_funcs": false + } + }, + "props": { + "cname": 6, + "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": "", - "$_": "" - } - } + "$_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" + } + } } From 32a7c7fe692f1fc617f8a20637889033291b6a48 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Fri, 16 Sep 2022 07:09:44 +0000 Subject: [PATCH 54/66] Fix: hide Computed internals --- packages/core/src/index.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 0a09bf7d2..b50ede88c 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -310,9 +310,16 @@ function returnComputed(computed: Computed): T { } class Computed extends Signal { + /** @internal */ _compute: () => T; + + /** @internal */ _sources?: Node = undefined; + + /** @internal */ _globalVersion = globalVersion - 1; + + /** @internal */ _flags = STALE; constructor(compute: () => T) { @@ -320,6 +327,7 @@ class Computed extends Signal { this._compute = compute; } + /** @internal */ _subscribe(node: Node) { if (this._targets === undefined) { this._flags |= STALE | SHOULD_SUBSCRIBE; @@ -333,6 +341,7 @@ class Computed extends Signal { super._subscribe(node); } + /** @internal */ _unsubscribe(node: Node) { super._unsubscribe(node) @@ -346,6 +355,7 @@ class Computed extends Signal { } } + /** @internal */ _notify() { if (!(this._flags & NOTIFIED)) { this._flags |= STALE | NOTIFIED; From 70bc14a0340836e609f31fa6a67958e43577f51b Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Fri, 16 Sep 2022 07:36:17 +0000 Subject: [PATCH 55/66] Add tests for effect disposal --- packages/core/src/index.ts | 21 +++++++++++++-------- packages/core/test/signal.test.tsx | 28 ++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index b50ede88c..9019b9c6a 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -8,6 +8,7 @@ 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. @@ -58,14 +59,16 @@ function endBatch() { while (effect !== undefined) { const next: Effect | undefined = effect._nextEffect; - effect._nextEffect = undefined; - effect._flags &= ~NOTIFIED; - try { - effect._callback(); - } catch (err) { - if (!hasError) { - error = err; - hasError = true; + if (!(effect._flags & DISPOSED)) { + effect._nextEffect = undefined; + effect._flags &= ~NOTIFIED; + try { + effect._callback(); + } catch (err) { + if (!hasError) { + error = err; + hasError = true; + } } } effect = next; @@ -477,6 +480,7 @@ class Effect { cycleDetected(); } this._flags |= RUNNING; + this._flags &= ~DISPOSED; /*@__INLINE__**/ startBatch(); const prevContext = evalContext; @@ -500,6 +504,7 @@ class Effect { node._source._unsubscribe(node); } this._sources = undefined; + this._flags |= DISPOSED; } } diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index e9bfa6012..ef1e3cd1e 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -160,6 +160,34 @@ describe("effect()", () => { expect(fn).to.throw(/Cycle detected/); }); + + 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; + }); }); describe("computed()", () => { From 0834499a709e3939f006cd74670b997dcca75820 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Fri, 16 Sep 2022 07:41:27 +0000 Subject: [PATCH 56/66] Throw when disposing a running effect --- packages/core/src/index.ts | 3 +++ packages/core/test/signal.test.tsx | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 9019b9c6a..9bcc8ddfe 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -500,6 +500,9 @@ class Effect { } _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); } diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index ef1e3cd1e..730ddcbf0 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -161,6 +161,24 @@ describe("effect()", () => { expect(fn).to.throw(/Cycle detected/); }); + 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); From 8aad3a16fcede554e4f93e50f42e0a601ce74d0f Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Fri, 16 Sep 2022 08:05:25 +0000 Subject: [PATCH 57/66] Fix: always reset effect state in batch handler --- packages/core/src/index.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 9bcc8ddfe..7bc94e1d7 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -59,9 +59,10 @@ function endBatch() { while (effect !== undefined) { const next: Effect | undefined = effect._nextEffect; + effect._nextEffect = undefined; + effect._flags &= ~NOTIFIED; + if (!(effect._flags & DISPOSED)) { - effect._nextEffect = undefined; - effect._flags &= ~NOTIFIED; try { effect._callback(); } catch (err) { From 2bfe375344b8f8e0b0ce9ff1575aca0cdea8d173 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Fri, 16 Sep 2022 08:18:10 +0000 Subject: [PATCH 58/66] Add core tests for internals that the integrations depend on --- packages/core/src/index.ts | 4 + packages/core/test/signal.test.tsx | 130 +++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 7bc94e1d7..e9bd6f7e5 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -449,6 +449,10 @@ export function computed(compute: () => T): Computed { export type { Computed as ReadonlySignal }; function endEffect(this: Effect, prevContext?: Computed | Effect) { + if (evalContext !== this) { + throw new Error("Out-of-order effect"); + } + cleanupSources(this); evalContext = prevContext; diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index 730ddcbf0..78efd904c 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -206,6 +206,136 @@ describe("effect()", () => { 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("_start returns a function for closing the effect scope", () => { + 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("___", () => { + 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; + }); + }); }); describe("computed()", () => { From 212d0c108a75b255bc19e325c38b9801d8c3a827 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Fri, 16 Sep 2022 08:27:25 +0000 Subject: [PATCH 59/66] Add test for internal effect._sources behavior --- packages/core/src/bench.ts | 19 ++++++++++++++++ packages/core/test/signal.test.tsx | 36 ++++++++++++++++++++++++++++-- 2 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 packages/core/src/bench.ts diff --git a/packages/core/src/bench.ts b/packages/core/src/bench.ts new file mode 100644 index 000000000..d5450714e --- /dev/null +++ b/packages/core/src/bench.ts @@ -0,0 +1,19 @@ +import * as core from "./index"; + +{ + const count = core.signal(0); + const double = core.computed(() => count.value * 2); + + core.effect(() => double.value + count.value); + core.effect(() => double.value + count.value); + core.effect(() => double.value + count.value); + + console.time("core"); + + for (let i = 0; i < 20000000; i++) { + count.value++; + double.value; + } + + console.timeEnd("core"); +} diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index 78efd904c..92e5438d1 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -236,7 +236,7 @@ describe("effect()", () => { expect(newSpy).to.be.called; }); - it("_start returns a function for closing the effect scope", () => { + it("should returns a function for closing the effect scope from _start", () => { const s = signal(0); let e: any; @@ -261,7 +261,7 @@ describe("effect()", () => { expect(spy).not.to.be.called; }); - it("___", () => { + it("should throw on out-of-order start1-start2-end1 sequences", () => { let e1: any; effect(function (this: any) { e1 = this; }); @@ -335,6 +335,38 @@ describe("effect()", () => { 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; + }); }); }); From b5ed584a4e7a187568c421b0a9652d7841053ca6 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Fri, 16 Sep 2022 08:29:20 +0000 Subject: [PATCH 60/66] Fix core tests --- packages/core/test/signal.test.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index 92e5438d1..07a20e386 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -355,7 +355,6 @@ describe("effect()", () => { done2(); expect(e._sources).to.be.undefined; - const done3 = e._start(); try { s.value; @@ -612,7 +611,7 @@ describe("computed()", () => { compute.resetHistory(); a.value = 4; - e.value; + d.value; expect(compute).to.have.been.calledOnce; }); From f3c2c198952350753fa0eb256a8a28ad2fb33655 Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Fri, 16 Sep 2022 12:46:28 +0200 Subject: [PATCH 61/66] Fix mangle.json formatting --- mangle.json | 114 ++++++++++++++++++++++++++-------------------------- 1 file changed, 57 insertions(+), 57 deletions(-) diff --git a/mangle.json b/mangle.json index 9dd9f456d..c48393bde 100644 --- a/mangle.json +++ b/mangle.json @@ -1,59 +1,59 @@ { - "help": { - "what is this file?": "It controls protected/private property mangling so that minified builds have consistent property names.", - "why are there duplicate minified properties?": "Most properties are only used on one type of objects, so they can have the same name since they will never collide. Doing this reduces size." - }, - "minify": { - "mangle": { - "keep_classnames": true, - "properties": { - "regex": "^_[^_]", - "reserved": [ - "__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED", - "__REACT_DEVTOOLS_GLOBAL_HOOK__", - "__PREACT_DEVTOOLS__", - "_renderers", - "_" - ] - } - }, - "compress": { - "reduce_funcs": false - } - }, - "props": { - "cname": 6, - "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" - } - } + "help": { + "what is this file?": "It controls protected/private property mangling so that minified builds have consistent property names.", + "why are there duplicate minified properties?": "Most properties are only used on one type of objects, so they can have the same name since they will never collide. Doing this reduces size." + }, + "minify": { + "mangle": { + "keep_classnames": true, + "properties": { + "regex": "^_[^_]", + "reserved": [ + "__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED", + "__REACT_DEVTOOLS_GLOBAL_HOOK__", + "__PREACT_DEVTOOLS__", + "_renderers", + "_" + ] + } + }, + "compress": { + "reduce_funcs": false + } + }, + "props": { + "cname": 6, + "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" + } + } } From d6c3bee71326423b744a293d902992c6025eede1 Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Fri, 16 Sep 2022 09:12:58 -0400 Subject: [PATCH 62/66] Prettier and slight optimization for Signal.toString --- packages/core/src/index.ts | 50 +++++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 11 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index e9bd6f7e5..9119f66c4 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -120,7 +120,7 @@ function getValue(signal: Signal): T { _source: signal, _nextSource: evalContext._sources, _target: evalContext, - _rollbackNode: node + _rollbackNode: node, }; evalContext._sources = node; signal._node = node; @@ -225,7 +225,7 @@ export class Signal { } toString(): string { - return "" + this.value; + return this.value + ""; } peek(): T { @@ -248,7 +248,11 @@ export class Signal { /**@__INLINE__*/ startBatch(); try { - for (let node = this._targets; node !== undefined; node = node._nextTarget) { + for ( + let node = this._targets; + node !== undefined; + node = node._nextTarget + ) { node._target._notify(); } } finally { @@ -263,7 +267,11 @@ export function signal(value: T): Signal { } function prepareSources(target: Computed | Effect) { - for (let node = target._sources; node !== undefined; node = node._nextSource) { + for ( + let node = target._sources; + node !== undefined; + node = node._nextSource + ) { const rollbackNode = node._source._node; if (rollbackNode !== undefined) { node._rollbackNode = rollbackNode; @@ -338,7 +346,11 @@ class Computed extends Signal { // 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) { + for ( + let node = this._sources; + node !== undefined; + node = node._nextSource + ) { node._source._subscribe(node); } } @@ -347,13 +359,17 @@ class Computed extends Signal { /** @internal */ _unsubscribe(node: Node) { - super._unsubscribe(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) { + for ( + let node = this._sources; + node !== undefined; + node = node._nextSource + ) { node._source._unsubscribe(node); } } @@ -364,7 +380,11 @@ class Computed extends Signal { if (!(this._flags & NOTIFIED)) { this._flags |= STALE | NOTIFIED; - for (let node = this._targets; node !== undefined; node = node._nextTarget) { + for ( + let node = this._targets; + node !== undefined; + node = node._nextTarget + ) { node._target._notify(); } } @@ -421,7 +441,11 @@ class Computed extends Signal { prepareSources(this); this._value = this._compute(); this._flags &= ~HAS_ERROR; - if ((prevFlags & HAS_ERROR) || this._value !== prevValue || this._version === 0) { + if ( + prevFlags & HAS_ERROR || + this._value !== prevValue || + this._version === 0 + ) { this._version++; } } catch (err) { @@ -429,7 +453,7 @@ class Computed extends Signal { this._flags |= HAS_ERROR; this._version++; } finally { - cleanupSources(this) + cleanupSources(this); evalContext = prevContext; } return returnComputed(this); @@ -508,7 +532,11 @@ class Effect { if (this._flags & RUNNING) { throw new Error("Effect still running"); } - for (let node = this._sources; node !== undefined; node = node._nextSource) { + for ( + let node = this._sources; + node !== undefined; + node = node._nextSource + ) { node._source._unsubscribe(node); } this._sources = undefined; From 9d91d3eb278fdd5a089e3cc0feb3cf52a3582d1b Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Fri, 16 Sep 2022 13:27:02 +0000 Subject: [PATCH 63/66] Remove an accidentally added scratchpad file --- packages/core/src/bench.ts | 19 ------------------- 1 file changed, 19 deletions(-) delete mode 100644 packages/core/src/bench.ts diff --git a/packages/core/src/bench.ts b/packages/core/src/bench.ts deleted file mode 100644 index d5450714e..000000000 --- a/packages/core/src/bench.ts +++ /dev/null @@ -1,19 +0,0 @@ -import * as core from "./index"; - -{ - const count = core.signal(0); - const double = core.computed(() => count.value * 2); - - core.effect(() => double.value + count.value); - core.effect(() => double.value + count.value); - core.effect(() => double.value + count.value); - - console.time("core"); - - for (let i = 0; i < 20000000; i++) { - count.value++; - double.value; - } - - console.timeEnd("core"); -} From 71d3fbeacb997515adb4afea530abd9d6d727625 Mon Sep 17 00:00:00 2001 From: Joachim Viide Date: Fri, 16 Sep 2022 13:17:50 +0000 Subject: [PATCH 64/66] Support auto-disposing nested effects --- packages/core/src/index.ts | 35 +++++++++-- packages/core/test/signal.test.tsx | 94 ++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 6 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index e9bd6f7e5..272d0bd2c 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -58,8 +58,8 @@ function endBatch() { batchIteration++; while (effect !== undefined) { - const next: Effect | undefined = effect._nextEffect; - effect._nextEffect = undefined; + const next: Effect | undefined = effect._nextBatchedEffect; + effect._nextBatchedEffect = undefined; effect._flags &= ~NOTIFIED; if (!(effect._flags & DISPOSED)) { @@ -313,6 +313,17 @@ function returnComputed(computed: Computed): T { return computed._value as T; } +function disposeNestedEffects(context: Computed | Effect) { + let effect = context._effects; + if (effect !== undefined) { + do { + effect._dispose(); + effect = effect._nextNestedEffect; + } while (effect !== undefined); + context._effects = undefined; + } +} + class Computed extends Signal { /** @internal */ _compute: () => T; @@ -320,6 +331,9 @@ class Computed extends Signal { /** @internal */ _sources?: Node = undefined; + /** @internal */ + _effects?: Effect = undefined; + /** @internal */ _globalVersion = globalVersion - 1; @@ -413,6 +427,8 @@ class Computed extends Signal { } } + disposeNestedEffects(this); + const prevValue = this._value; const prevFlags = this._flags; const prevContext = evalContext; @@ -463,8 +479,10 @@ function endEffect(this: Effect, prevContext?: Computed | Effect) { class Effect { _compute: () => void; - _sources?: Node = undefined; - _nextEffect?: Effect = undefined; + _sources?: Node; + _effects?: Effect; + _nextNestedEffect?: Effect; + _nextBatchedEffect?: Effect; _flags = SHOULD_SUBSCRIBE; constructor(compute: () => void) { @@ -486,10 +504,14 @@ class Effect { } 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; prepareSources(this); @@ -499,7 +521,7 @@ class Effect { _notify() { if (!(this._flags & NOTIFIED)) { this._flags |= NOTIFIED; - this._nextEffect = batchedEffect; + this._nextBatchedEffect = batchedEffect; batchedEffect = this; } } @@ -511,6 +533,7 @@ class Effect { for (let node = this._sources; node !== undefined; node = node._nextSource) { node._source._unsubscribe(node); } + disposeNestedEffects(this); this._sources = undefined; this._flags |= DISPOSED; } diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index 91ea597e7..c1d10b640 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -161,6 +161,74 @@ 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(); @@ -422,6 +490,32 @@ describe("computed()", () => { 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 { From a367a317fac9546192aaee67e66d6f328b45ec1e Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Fri, 16 Sep 2022 15:36:23 +0200 Subject: [PATCH 65/66] Add changeset --- .changeset/heavy-turkeys-relate.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/heavy-turkeys-relate.md 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 From 6ac6923e5294f8a31ee1a009550b9891c3996cb4 Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Fri, 16 Sep 2022 16:40:56 +0200 Subject: [PATCH 66/66] Add core changeset --- .changeset/shiny-jars-invite.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/shiny-jars-invite.md 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.