Skip to content

Commit

Permalink
Merge pull request #558 from jviide/patch-01
Browse files Browse the repository at this point in the history 
fix(core): Restore stricter effect callback & cleanup function types
  • Loading branch information
@marvinhagemeister
marvinhagemeister committed Apr 13, 2024
2 parents 8bb9192 + c8c95ac commit bd5c4d8
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 39 deletions.
5 changes: 5 additions & 0 deletions .changeset/nine-fishes-leave.md
View file
@@ -0,0 +1,5 @@
---
"@preact/signals-core": patch
---

Restore stricter effect callback & cleanup function types
14 changes: 8 additions & 6 deletions packages/core/src/index.ts
View file
Expand Up @@ -724,22 +724,24 @@ function endEffect(this: Effect, prevContext?: Computed | Effect) {
endBatch();
}

type EffectFn = () => void | (() => void);

declare class Effect {
_fn?: () => unknown;
_cleanup?: () => unknown;
_fn?: EffectFn;
_cleanup?: () => void;
_sources?: Node;
_nextBatchedEffect?: Effect;
_flags: number;

constructor(fn: () => unknown);
constructor(fn: EffectFn);

_callback(): void;
_start(): () => void;
_notify(): void;
_dispose(): void;
}

function Effect(this: Effect, fn: () => unknown) {
function Effect(this: Effect, fn: EffectFn) {
this._fn = fn;
this._cleanup = undefined;
this._sources = undefined;
Expand All @@ -755,7 +757,7 @@ Effect.prototype._callback = function () {

const cleanup = this._fn();
if (typeof cleanup === "function") {
this._cleanup = cleanup as () => unknown;
this._cleanup = cleanup;
}
} finally {
finish();
Expand Down Expand Up @@ -806,7 +808,7 @@ Effect.prototype._dispose = function () {
* @param fn The effect callback.
* @returns A function for disposing the effect.
*/
function effect(fn: () => unknown): () => void {
function effect(fn: EffectFn): () => void {
const effect = new Effect(fn);
try {
effect._callback();
Expand Down
120 changes: 87 additions & 33 deletions packages/core/test/signal.test.tsx
View file
Expand Up @@ -48,9 +48,15 @@ describe("signal", () => {

it("should notify other listeners of changes after one listener is disposed", () => {
const s = signal(0);
const spy1 = sinon.spy(() => s.value);
const spy2 = sinon.spy(() => s.value);
const spy3 = sinon.spy(() => s.value);
const spy1 = sinon.spy(() => {
s.value;
});
const spy2 = sinon.spy(() => {
s.value;
});
const spy3 = sinon.spy(() => {
s.value;
});

effect(spy1);
const dispose = effect(spy2);
Expand Down Expand Up @@ -187,42 +193,48 @@ describe("signal", () => {
});

describe("effect()", () => {
it("should init with value", () => {
it("should run the callback immediately", () => {
const s = signal(123);
const spy = sinon.spy(() => s.value);
const spy = sinon.spy(() => {
s.value;
});
effect(spy);

expect(spy).to.be.called;
expect(spy).to.returned(123);
});

it("should subscribe to signals", () => {
const s = signal(123);
const spy = sinon.spy(() => s.value);
const spy = sinon.spy(() => {
s.value;
});
effect(spy);
spy.resetHistory();

s.value = 42;
expect(spy).to.be.called;
expect(spy).to.returned(42);
});

it("should subscribe to multiple signals", () => {
const a = signal("a");
const b = signal("b");
const spy = sinon.spy(() => a.value + " " + b.value);
const spy = sinon.spy(() => {
a.value;
b.value;
});
effect(spy);
spy.resetHistory();

a.value = "aa";
b.value = "bb";
expect(spy).to.returned("aa bb");
expect(spy).to.be.calledTwice;
});

it("should dispose of subscriptions", () => {
const a = signal("a");
const b = signal("b");
const spy = sinon.spy(() => a.value + " " + b.value);
const spy = sinon.spy(() => {
a.value + " " + b.value;
});
const dispose = effect(spy);
spy.resetHistory();

Expand All @@ -236,7 +248,9 @@ describe("effect()", () => {

it("should unsubscribe from signal", () => {
const s = signal(123);
const spy = sinon.spy(() => s.value);
const spy = sinon.spy(() => {
s.value;
});
const unsub = effect(spy);
spy.resetHistory();

Expand All @@ -251,7 +265,7 @@ describe("effect()", () => {
const cond = signal(true);

const spy = sinon.spy(() => {
return cond.value ? a.value : b.value;
cond.value ? a.value : b.value;
});

effect(spy);
Expand All @@ -271,7 +285,9 @@ describe("effect()", () => {

it("should batch writes", () => {
const a = signal("a");
const spy = sinon.spy(() => a.value);
const spy = sinon.spy(() => {
a.value;
});
effect(spy);
spy.resetHistory();

Expand Down Expand Up @@ -642,7 +658,9 @@ describe("effect()", () => {

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 spy = sinon.spy(() => {
a.value;
});
const dispose = effect(spy);
spy.resetHistory();

Expand All @@ -656,7 +674,9 @@ describe("effect()", () => {

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 spy = sinon.spy(() => {
a.value;
});
const dispose = effect(spy);
spy.resetHistory();

Expand All @@ -673,8 +693,12 @@ describe("effect()", () => {
const parentSignal = signal(0);
const childSignal = signal(0);

const parentEffect = sinon.spy(() => parentSignal.value);
const childEffect = sinon.spy(() => childSignal.value);
const parentEffect = sinon.spy(() => {
parentSignal.value;
});
const childEffect = sinon.spy(() => {
childSignal.value;
});

effect(() => {
parentEffect();
Expand Down Expand Up @@ -1102,9 +1126,15 @@ describe("computed()", () => {
const s = signal(0);
const c = computed(() => s.value);

const spy1 = sinon.spy(() => c.value);
const spy2 = sinon.spy(() => c.value);
const spy3 = sinon.spy(() => c.value);
const spy1 = sinon.spy(() => {
c.value;
});
const spy2 = sinon.spy(() => {
c.value;
});
const spy3 = sinon.spy(() => {
c.value;
});

effect(spy1);
const dispose = effect(spy2);
Expand Down Expand Up @@ -1314,7 +1344,9 @@ describe("computed()", () => {
(function () {
const c = computed(() => s.value);
ref = new WeakRef(c);
dispose = effect(() => c.value);
dispose = effect(() => {
c.value;
});
})();

dispose();
Expand Down Expand Up @@ -1545,7 +1577,9 @@ describe("computed()", () => {
const d = computed(() => a.value);

let result = "";
const unsub = effect(() => (result = c.value));
const unsub = effect(() => {
result = c.value;
});

expect(result).to.equal("a");
expect(d.value).to.equal("a");
Expand Down Expand Up @@ -1721,7 +1755,9 @@ describe("batch/transaction", () => {
it("should delay writes", () => {
const a = signal("a");
const b = signal("b");
const spy = sinon.spy(() => a.value + " " + b.value);
const spy = sinon.spy(() => {
a.value + " " + b.value;
});
effect(spy);
spy.resetHistory();

Expand All @@ -1736,7 +1772,9 @@ describe("batch/transaction", () => {
it("should delay writes until outermost batch is complete", () => {
const a = signal("a");
const b = signal("b");
const spy = sinon.spy(() => a.value + ", " + b.value);
const spy = sinon.spy(() => {
a.value + ", " + b.value;
});
effect(spy);
spy.resetHistory();

Expand Down Expand Up @@ -1812,7 +1850,9 @@ describe("batch/transaction", () => {
const d = computed(() => a.value + " " + b.value + " " + c.value);

let result;
effect(() => (result = d.value));
effect(() => {
result = d.value;
});

batch(() => {
a.value = "aa";
Expand Down Expand Up @@ -1865,8 +1905,12 @@ describe("batch/transaction", () => {
it("should run pending effects even if the callback throws", () => {
const a = signal(0);
const b = signal(1);
const spy1 = sinon.spy(() => a.value);
const spy2 = sinon.spy(() => b.value);
const spy1 = sinon.spy(() => {
a.value;
});
const spy2 = sinon.spy(() => {
b.value;
});
effect(spy1);
effect(spy2);
spy1.resetHistory();
Expand All @@ -1886,8 +1930,12 @@ describe("batch/transaction", () => {

it("should run pending effects even if some effects throw", () => {
const a = signal(0);
const spy1 = sinon.spy(() => a.value);
const spy2 = sinon.spy(() => a.value);
const spy1 = sinon.spy(() => {
a.value;
});
const spy2 = sinon.spy(() => {
a.value;
});
effect(() => {
if (a.value === 1) {
throw new Error("hello");
Expand Down Expand Up @@ -1933,7 +1981,9 @@ describe("untracked", () => {
it("should block tracking inside effects", () => {
const a = signal(1);
const b = signal(2);
const spy = sinon.spy(() => a.value + b.value);
const spy = sinon.spy(() => {
a.value + b.value;
});
effect(() => untracked(spy));
expect(spy).to.be.calledOnce;

Expand All @@ -1946,7 +1996,11 @@ describe("untracked", () => {
const s = signal(1);
const spy = sinon.spy(() => s.value);

untracked(() => effect(() => untracked(spy)));
untracked(() =>
effect(() => {
untracked(spy);
})
);
expect(spy).to.be.calledOnce;

s.value = 2;
Expand Down

0 comments on commit bd5c4d8

Please sign in to comment.