diff --git a/.changeset/tidy-buses-do.md b/.changeset/tidy-buses-do.md new file mode 100644 index 000000000..74bc36bf6 --- /dev/null +++ b/.changeset/tidy-buses-do.md @@ -0,0 +1,16 @@ +--- +"@preact/signals-core": minor +"@preact/signals": minor +"@preact/signals-react": minor +--- + +Add ability to run custom cleanup logic when an effect is disposed. + +```js +effect(() => { + console.log("This runs whenever a dependency changes"); + return () => { + console.log("This runs when the effect is disposed"); + }); +}); +``` diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 55e85ccb2..1585f9596 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -2,20 +2,26 @@ function cycleDetected(): never { throw new Error("Cycle detected"); } +// Flags for Computed and Effect. const RUNNING = 1 << 0; -const STALE = 1 << 1; -const NOTIFIED = 1 << 2; -const HAS_ERROR = 1 << 3; -const SHOULD_SUBSCRIBE = 1 << 4; -const SUBSCRIBED = 1 << 5; -const DISPOSED = 1 << 6; +const NOTIFIED = 1 << 1; +const OUTDATED = 1 << 2; +const DISPOSED = 1 << 3; +const HAS_ERROR = 1 << 4; +const IS_EFFECT = 1 << 5; +const AUTO_DISPOSE = 1 << 6; +const AUTO_SUBSCRIBE = 1 << 7; + +// Flags for Nodes. +const NODE_FREE = 1 << 0; +const NODE_SUBSCRIBED = 1 << 1; // A linked list node used to track dependencies (sources) and dependents (targets). // Also used to remember the source's last version number that the target saw. type Node = { // A node may have the following flags: - // SUBSCRIBED when the target has subscribed to listen change notifications from the source - // STALE when it's unclear whether the source is still a dependency of the target + // NODE_FREE when it's unclear whether the source is still a dependency of the target + // NODE_SUBSCRIBED when the target has subscribed to listen change notifications from the source _flags: number; // A source whose value the target depends on. @@ -62,7 +68,7 @@ function endBatch() { effect._nextBatchedEffect = undefined; effect._flags &= ~NOTIFIED; - if (!(effect._flags & DISPOSED)) { + if (!(effect._flags & DISPOSED) && (effect._flags & OUTDATED)) { try { effect._callback(); } catch (err) { @@ -132,14 +138,14 @@ function addDependency(signal: Signal): Node | undefined { // Subscribe to change notifications from this dependency if we're in an effect // OR evaluating a computed signal that in turn has subscribers. - if (evalContext._flags & SHOULD_SUBSCRIBE) { + if (evalContext._flags & AUTO_SUBSCRIBE) { signal._subscribe(node); } return node; - } else if (node._flags & STALE) { + } else if (node._flags & NODE_FREE) { // `signal` is an existing dependency from a previous evaluation. Reuse the dependency // node and move it to the front of the evaluation context's dependency list. - node._flags &= ~STALE; + node._flags &= ~NODE_FREE; const head = evalContext._sources; if (node !== head) { @@ -216,8 +222,8 @@ Signal.prototype._refresh = function() { }; Signal.prototype._subscribe = function(node) { - if (!(node._flags & SUBSCRIBED)) { - node._flags |= SUBSCRIBED; + if (!(node._flags & NODE_SUBSCRIBED)) { + node._flags |= NODE_SUBSCRIBED; node._nextTarget = this._targets; if (this._targets !== undefined) { @@ -228,8 +234,8 @@ Signal.prototype._subscribe = function(node) { }; Signal.prototype._unsubscribe = function(node) { - if (node._flags & SUBSCRIBED) { - node._flags &= ~SUBSCRIBED; + if (node._flags & NODE_SUBSCRIBED) { + node._flags &= ~NODE_SUBSCRIBED; const prev = node._prevTarget; const next = node._nextTarget; @@ -312,7 +318,7 @@ function prepareSources(target: Computed | Effect) { node._rollbackNode = rollbackNode; } node._source._node = node; - node._flags |= STALE; + node._flags |= NODE_FREE; } } @@ -327,7 +333,7 @@ function cleanupSources(target: Computed | Effect) { let sources = undefined; while (node !== undefined) { const next = node._nextSource; - if (node._flags & STALE) { + if (node._flags & NODE_FREE) { node._source._unsubscribe(node); node._nextSource = undefined; } else { @@ -348,14 +354,51 @@ function cleanupSources(target: Computed | Effect) { target._sources = sources; } -function disposeNestedEffects(context: Computed | Effect) { - let effect = context._effects; - if (effect !== undefined) { - do { - effect._dispose(); - effect = effect._nextNestedEffect; - } while (effect !== undefined); +function cleanupContext(context: Computed | Effect) { + let hasError = false; + let error: unknown; + + let nested = context._effects; + if (nested !== undefined) { context._effects = undefined; + + while (nested !== undefined) { + try { + nested._dispose(); + } catch (err) { + hasError = true; + error = err; + } + nested = nested._nextNestedEffect; + } + } + + if (context._flags & IS_EFFECT) { + const cleanup = (context as Effect)._cleanup; + (context as Effect)._cleanup = undefined; + + if (typeof cleanup === "function") { + /*@__INLINE__**/ startBatch(); + + // Run cleanup functions always outside of any context. + const prevContext = evalContext; + evalContext = undefined; + + try { + cleanup(); + } catch (err) { + hasError = true; + error = err; + context._flags &= ~RUNNING; + } + + evalContext = prevContext; + endBatch(); + } + } + + if (hasError) { + throw error; } } @@ -391,7 +434,7 @@ function Computed(this: Computed, compute: () => unknown) { this._sources = undefined; this._effects = undefined; this._globalVersion = globalVersion - 1; - this._flags = STALE; + this._flags = OUTDATED; } Computed.prototype = new Signal() as Computed; @@ -403,10 +446,12 @@ Computed.prototype._refresh = function() { return false; } - if (!(this._flags & STALE) && this._targets !== undefined) { + // Trust the OUTDATED flag only when the computed signal has subscribed + // to any notifications from dependencies. + if (this._targets !== undefined && !(this._flags & OUTDATED)) { return true; } - this._flags &= ~STALE; + this._flags &= ~OUTDATED; if (this._globalVersion === globalVersion) { return true; @@ -436,7 +481,7 @@ Computed.prototype._refresh = function() { } prepareSources(this); - disposeNestedEffects(this); + cleanupContext(this); evalContext = this; const value = this._compute(); @@ -463,7 +508,7 @@ Computed.prototype._refresh = function() { Computed.prototype._subscribe = function(node) { if (this._targets === undefined) { - this._flags |= STALE | SHOULD_SUBSCRIBE; + this._flags |= OUTDATED | AUTO_SUBSCRIBE; // A computed signal subscribes lazily to its dependencies when the it // gets its first subscriber. @@ -483,7 +528,7 @@ Computed.prototype._unsubscribe = function(node) { // Computed signal unsubscribes from its dependencies from it loses its last subscriber. if (this._targets === undefined) { - this._flags &= ~SHOULD_SUBSCRIBE; + this._flags &= ~AUTO_SUBSCRIBE; for ( let node = this._sources; @@ -497,7 +542,7 @@ Computed.prototype._unsubscribe = function(node) { Computed.prototype._notify = function() { if (!(this._flags & NOTIFIED)) { - this._flags |= STALE | NOTIFIED; + this._flags |= OUTDATED | NOTIFIED; for ( let node = this._targets; @@ -548,9 +593,9 @@ function disposeEffect(effect: Effect) { ) { node._source._unsubscribe(node); } - disposeNestedEffects(effect); effect._sources = undefined; - effect._flags |= DISPOSED; + + cleanupContext(effect); } function endEffect(this: Effect, prevContext?: Computed | Effect) { @@ -559,23 +604,24 @@ function endEffect(this: Effect, prevContext?: Computed | Effect) { } cleanupSources(this); evalContext = prevContext; - endBatch(); this._flags &= ~RUNNING; if (this._flags & DISPOSED) { disposeEffect(this); } + endBatch(); } declare class Effect { - _compute: () => void; + _compute: () => unknown; + _cleanup?: unknown; _sources?: Node; _effects?: Effect; _nextNestedEffect?: Effect; _nextBatchedEffect?: Effect; _flags: number; - constructor(compute: () => void); + constructor(compute: () => void, flags: number); _callback(): void; _start(): () => void; @@ -583,15 +629,16 @@ declare class Effect { _dispose(): void; } -function Effect(this: Effect, compute: () => void) { +function Effect(this: Effect, compute: () => void, flags: number) { this._compute = compute; + this._cleanup = undefined; this._sources = undefined; this._effects = undefined; this._nextNestedEffect = undefined; this._nextBatchedEffect = undefined; - this._flags = SHOULD_SUBSCRIBE; + this._flags = IS_EFFECT | OUTDATED | flags; - if (evalContext !== undefined) { + if ((flags & AUTO_DISPOSE) && evalContext !== undefined) { this._nextNestedEffect = evalContext._effects; evalContext._effects = this; } @@ -600,7 +647,9 @@ function Effect(this: Effect, compute: () => void) { Effect.prototype._callback = function() { const finish = this._start(); try { - this._compute(); + if (!(this._flags & DISPOSED)) { + this._cleanup = this._compute(); + } } finally { finish(); } @@ -612,32 +661,34 @@ Effect.prototype._start = function() { } this._flags |= RUNNING; this._flags &= ~DISPOSED; - disposeNestedEffects(this); + prepareSources(this); + cleanupContext(this); /*@__INLINE__**/ startBatch(); + this._flags &= ~OUTDATED; const prevContext = evalContext; evalContext = this; - - prepareSources(this); return endEffect.bind(this, prevContext); }; Effect.prototype._notify = function() { if (!(this._flags & NOTIFIED)) { - this._flags |= NOTIFIED; + this._flags |= NOTIFIED | OUTDATED; this._nextBatchedEffect = batchedEffect; batchedEffect = this; } }; Effect.prototype._dispose = function() { + this._flags |= DISPOSED; + if (!(this._flags & RUNNING)) { disposeEffect(this); } }; -function effect(compute: () => void): () => void { - const effect = new Effect(compute); +function effect(compute: () => unknown): () => void { + const effect = new Effect(compute, AUTO_DISPOSE | AUTO_SUBSCRIBE); 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. diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index b92bd3af7..cf377bc38 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -161,6 +161,260 @@ describe("effect()", () => { expect(spy).to.be.calledOnce; }); + it("should call the cleanup callback before the next run", () => { + const a = signal(0); + const spy = sinon.spy(); + + effect(() => { + a.value; + return spy; + }); + expect(spy).not.to.be.called; + a.value = 1; + expect(spy).to.be.calledOnce; + a.value = 2; + expect(spy).to.be.calledTwice; + }); + + it("should call only the callback from the previous run", () => { + const spy1 = sinon.spy(); + const spy2 = sinon.spy(); + const spy3 = sinon.spy(); + const a = signal(spy1); + + effect(() => { + return a.value; + }); + + expect(spy1).not.to.be.called; + expect(spy2).not.to.be.called; + expect(spy3).not.to.be.called; + + a.value = spy2; + expect(spy1).to.be.calledOnce; + expect(spy2).not.to.be.called; + expect(spy3).not.to.be.called; + + a.value = spy3; + expect(spy1).to.be.calledOnce; + expect(spy2).to.be.calledOnce; + expect(spy3).not.to.be.called; + }); + + it("should call the cleanup callback function when disposed", () => { + const spy = sinon.spy(); + + const dispose = effect(() => { + return spy; + }); + expect(spy).not.to.be.called; + dispose(); + expect(spy).to.be.calledOnce; + }); + + it("should run the cleanup in an implicit batch", () => { + const a = signal(0); + const b = signal("a"); + const c = signal("b"); + const spy = sinon.spy(); + + effect(() => { + b.value; + c.value; + spy(b.value + c.value); + }); + + effect(() => { + a.value; + return () => { + b.value = "x"; + c.value = "y"; + }; + }); + + expect(spy).to.be.calledOnce; + spy.resetHistory(); + + a.value = 1; + expect(spy).to.be.calledOnce; + expect(spy).to.be.calledWith("xy"); + }); + + it("should not retrigger the effect if the cleanup modifies one of the dependencies", () => { + const a = signal(0); + const spy = sinon.spy(); + + effect(() => { + spy(a.value); + return () => { + a.value = 2; + }; + }); + expect(spy).to.be.calledOnce; + spy.resetHistory(); + + a.value = 1; + expect(spy).to.be.calledOnce; + expect(spy).to.be.calledWith(2); + }); + + it("should run the cleanup if the effect disposes itself", () => { + const a = signal(0); + const spy = sinon.spy(); + + const dispose = effect(() => { + if (a.value > 0) { + dispose(); + return spy; + } + }); + expect(spy).not.to.be.called; + a.value = 1; + expect(spy).to.be.calledOnce; + a.value = 2; + expect(spy).to.be.calledOnce; + }); + + it("should not run the effect if the cleanup function disposes it", () => { + const a = signal(0); + const spy = sinon.spy(); + + const dispose = effect(() => { + a.value; + spy(); + return () => { + dispose(); + }; + }); + expect(spy).to.be.calledOnce; + a.value = 1; + expect(spy).to.be.calledOnce; + }); + + it("should reset the cleanup if the effect throws", () => { + const a = signal(0); + const spy = sinon.spy(); + + effect(() => { + if (a.value === 0) { + return spy; + } else { + throw new Error("hello"); + } + }); + expect(spy).not.to.be.called; + expect(() => a.value = 1).to.throw("hello"); + expect(spy).to.be.calledOnce; + a.value = 0; + expect(spy).to.be.calledOnce; + }); + + it("should dispose the effect if the cleanup callback throws", () => { + const a = signal(0); + const spy = sinon.spy(); + + effect(() => { + if (a.value === 0) { + return () => { + throw new Error("hello"); + } + } else { + spy(); + } + }); + expect(spy).not.to.be.called; + expect(() => a.value++).to.throw("hello"); + expect(spy).not.to.be.called; + }); + + it("should run cleanups outside any evaluation context", () => { + const spy = sinon.spy(); + const a = signal(0); + const b = signal(0); + const c = computed(() => { + if (a.value === 0) { + effect(() => { + return () => { + b.value; + }; + }); + } + }); + + effect(() => { + spy(); + c.value; + }); + expect(spy).to.be.calledOnce; + spy.resetHistory(); + + a.value = 1; + expect(spy).to.be.calledOnce; + spy.resetHistory(); + + b.value = 1; + expect(spy).not.to.be.called; + }); + + it("should run nested cleanups before their own", () => { + const spy1 = sinon.spy(); + const spy2 = sinon.spy(); + + const dispose = effect(() => { + effect(() => { + return spy1; + }); + return spy2; + }); + expect(spy1).not.to.be.called; + expect(spy2).not.to.be.called; + dispose(); + expect(spy2).to.be.calledAfter(spy1); + }); + + it("should run all cleanups even if some of them fail", () => { + const spy1 = sinon.spy(); + const spy2 = sinon.spy(); + + const dispose = effect(() => { + effect(() => { + return spy1; + }); + + effect(() => { + return () => { + throw new Error("hello"); + }; + }); + + effect(() => { + return spy2; + }); + }); + expect(spy1).not.to.be.called; + expect(spy2).not.to.be.called; + expect(dispose).to.throw("hello"); + expect(spy1).to.be.calledOnce; + expect(spy2).to.be.calledOnce; + }); + + it("should throw one of the errors thrown by cleanups if multiple cleanups fail", () => { + const dispose = effect(() => { + effect(() => { + return () => { + throw new Error("error 1"); + }; + }); + + effect(() => { + return () => { + throw new Error("error 2"); + }; + }); + }); + expect(dispose).to.throw(/error (1|2)/) + }); + it("should throw on cycles", () => { const a = signal(0); let i = 0; @@ -704,6 +958,123 @@ describe("computed()", () => { spy.resetHistory(); }); + it("should run the cleanups for nested effects", () => { + const spy = sinon.spy(); + const a = signal(0); + const c = computed(() => { + a.value; + + if (a.value === 0) { + effect(() => { + return spy; + }); + } + }); + + c.value; + expect(spy).not.to.be.called; + + a.value++; + c.value; + expect(spy).to.be.calledOnce; + + a.value++; + c.value; + expect(spy).to.be.calledOnce; + }); + + it("should adopt the failure when an effect cleanup fails before recomputation", () => { + const a = signal(0); + const spy = sinon.spy(); + const c = computed(() => { + spy(); + + a.value; + + effect(() => { + return () => { + throw new Error("hello"); + }; + }); + }); + + c.value; + expect(spy).to.be.calledOnce; + spy.resetHistory(); + + a.value++; + expect(() => c.value).to.throw("hello"); + expect(spy).not.to.be.called; + + expect(() => c.value).to.throw("hello"); + expect(spy).not.to.be.called; + }); + + it("should drop all dependencies if an effect cleanup before recomputation fails", () => { + const a = signal(0); + const b = signal(0); + const spy = sinon.spy(); + const c = computed(() => { + spy(); + + a.value; + + effect(() => { + return () => { + throw new Error("hello"); + }; + }); + + b.value; + }); + + c.value; + expect(spy).to.be.calledOnce; + spy.resetHistory(); + + a.value++; + expect(() => c.value).to.throw("hello"); + expect(spy).not.to.be.called; + + a.value++; + expect(() => c.value).to.throw("hello"); + expect(spy).not.to.be.called; + + b.value++; + expect(() => c.value).to.throw("hello"); + expect(spy).not.to.be.called; + }); + + it("should run all cleanups even if some of them fail", () => { + const spy1 = sinon.spy(); + const spy2 = sinon.spy(); + const a = signal(0); + const c = computed(() => { + a.value; + + effect(() => { + return spy1; + }); + + effect(() => { + return () => { + throw new Error("hello"); + }; + }); + + effect(() => { + return spy2; + }); + }); + c.value; + expect(spy1).not.to.be.called; + expect(spy2).not.to.be.called; + a.value++; + expect(() => c.value).to.throw("hello"); + expect(spy1).to.be.calledOnce; + expect(spy2).to.be.calledOnce; + }); + describe("graph updates", () => { it("should run computeds once for multiple dep changes", async () => { const a = signal("a"); diff --git a/packages/preact/src/index.ts b/packages/preact/src/index.ts index 187ace5ff..2b26e798b 100644 --- a/packages/preact/src/index.ts +++ b/packages/preact/src/index.ts @@ -316,17 +316,9 @@ export function useSignalEffect(cb: () => void | (() => void)) { callback.current = cb; useEffect(() => { - let cleanup: (() => void) | undefined; return effect(() => { - if (cleanup) { - cleanup(); - cleanup = undefined - } - const result = callback.current(); - if (typeof result == 'function') { - cleanup = result - } - }) + return callback.current(); + }); }, []); } diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 565edfd44..eb048eb14 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -169,16 +169,8 @@ export function useSignalEffect(cb: () => void | (() => void)) { callback.current = cb; useEffect(() => { - let cleanup: (() => void) | undefined; return effect(() => { - if (cleanup) { - cleanup(); - cleanup = undefined - } - const result = callback.current(); - if (typeof result == 'function') { - cleanup = result - } - }) + return callback.current(); + }); }, []); }