Skip to content

Commit 458f816

Browse files
developitdevelopit
authored
Merge pull request #282 from preactjs/fix-usesignaleffect-cleanup
Bugfix: useSignalEffect cleanup not called
2 parents 133bc1e + e1bb7a6 commit 458f816

File tree

5 files changed

+297
-11
lines changed

5 files changed

+297
-11
lines changed

‎.changeset/short-paws-listen.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@preact/signals": patch
3+
"@preact/signals-react": patch
4+
---
5+
6+
Fix a bug that caused cleanup functions returned from a `useSignalEffect()` callback not to be called.

‎packages/preact/src/index.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -347,9 +347,7 @@ export function useSignalEffect(cb: () => void | (() => void)) {
347347
callback.current = cb;
348348

349349
useEffect(() => {
350-
return effect(() => {
351-
callback.current();
352-
});
350+
return effect(() => callback.current());
353351
}, []);
354352
}
355353

‎packages/preact/test/index.test.tsx‎

Lines changed: 152 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
1-
import { signal, computed, useComputed, Signal } from "@preact/signals";
2-
import { createElement, render } from "preact";
1+
import {
2+
signal,
3+
computed,
4+
useComputed,
5+
useSignalEffect,
6+
Signal,
7+
} from "@preact/signals";
8+
import { createElement, createRef, render } from "preact";
39
import { setupRerender, act } from "preact/test-utils";
410

511
const sleep = (ms?: number) => new Promise(r => setTimeout(r, ms));
@@ -346,4 +352,148 @@ describe("@preact/signals", () => {
346352
});
347353
});
348354
});
355+
356+
describe("useSignalEffect()", () => {
357+
it("should be invoked after commit", async () => {
358+
const ref = createRef();
359+
const sig = signal("foo");
360+
const spy = sinon.spy();
361+
let count = 0;
362+
363+
function App() {
364+
useSignalEffect(() =>
365+
spy(
366+
sig.value,
367+
ref.current,
368+
ref.current.getAttribute("data-render-id")
369+
)
370+
);
371+
return (
372+
<p ref={ref} data-render-id={count++}>
373+
{sig.value}
374+
</p>
375+
);
376+
}
377+
378+
act(() => {
379+
render(<App />, scratch);
380+
});
381+
expect(scratch.textContent).to.equal("foo");
382+
// expect(spy).not.to.have.been.called;
383+
await sleep(1);
384+
expect(spy).to.have.been.calledOnceWith(
385+
"foo",
386+
scratch.firstElementChild,
387+
"0"
388+
);
389+
390+
spy.resetHistory();
391+
392+
act(() => {
393+
sig.value = "bar";
394+
rerender();
395+
});
396+
397+
expect(scratch.textContent).to.equal("bar");
398+
await sleep(1);
399+
400+
// NOTE: Ideally, call should receive "1" as its third argument!
401+
// The "0" indicates that Preact's DOM mutations hadn't yet been performed when the callback ran.
402+
// This happens because we do signal-based effect runs after the first, not VDOM.
403+
// Perhaps we could find a way to defer the callback when it coincides with a render?
404+
expect(spy).to.have.been.calledOnceWith(
405+
"bar",
406+
scratch.firstElementChild,
407+
"0" // ideally "1" - update if we find a nice way to do so!
408+
);
409+
});
410+
411+
it("should invoke any returned cleanup function for updates", async () => {
412+
const ref = createRef();
413+
const sig = signal("foo");
414+
const spy = sinon.spy();
415+
const cleanup = sinon.spy();
416+
let count = 0;
417+
418+
function App() {
419+
useSignalEffect(() => {
420+
const id = ref.current.getAttribute("data-render-id");
421+
const value = sig.value;
422+
spy(value, ref.current, id);
423+
return () => cleanup(value, ref.current, id);
424+
});
425+
return (
426+
<p ref={ref} data-render-id={count++}>
427+
{sig.value}
428+
</p>
429+
);
430+
}
431+
432+
render(<App />, scratch);
433+
434+
await sleep(1);
435+
expect(cleanup).not.to.have.been.called;
436+
expect(spy).to.have.been.calledOnceWith(
437+
"foo",
438+
scratch.firstElementChild,
439+
"0"
440+
);
441+
spy.resetHistory();
442+
443+
act(() => {
444+
sig.value = "bar";
445+
rerender();
446+
});
447+
448+
expect(scratch.textContent).to.equal("bar");
449+
await sleep(1);
450+
451+
const child = scratch.firstElementChild;
452+
453+
expect(cleanup).to.have.been.calledOnceWith("foo", child, "0");
454+
455+
expect(spy).to.have.been.calledOnceWith(
456+
"bar",
457+
child,
458+
"0" // ideally "1" - update if we find a nice way to do so!
459+
);
460+
});
461+
462+
it("should invoke any returned cleanup function for unmounts", async () => {
463+
const ref = createRef();
464+
const sig = signal("foo");
465+
const spy = sinon.spy();
466+
const cleanup = sinon.spy();
467+
468+
function App() {
469+
useSignalEffect(() => {
470+
const value = sig.value;
471+
spy(value, ref.current);
472+
return () => cleanup(value, ref.current);
473+
});
474+
return <p ref={ref}>{sig.value}</p>;
475+
}
476+
477+
act(() => {
478+
render(<App />, scratch);
479+
});
480+
481+
await sleep(1);
482+
483+
const child = scratch.firstElementChild;
484+
485+
expect(cleanup).not.to.have.been.called;
486+
expect(spy).to.have.been.calledOnceWith("foo", child);
487+
spy.resetHistory();
488+
489+
act(() => {
490+
render(null, scratch);
491+
});
492+
493+
await sleep(1);
494+
495+
expect(spy).not.to.have.been.called;
496+
expect(cleanup).to.have.been.calledOnceWith("foo", child);
497+
});
498+
});
349499
});

‎packages/react/src/index.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,10 @@ export { signal, computed, batch, effect, Signal, type ReadonlySignal };
2424
const Empty = [] as const;
2525
const ReactElemType = Symbol.for("react.element"); // https://github.com/facebook/react/blob/346c7d4c43a0717302d446da9e7423a8e28d8996/packages/shared/ReactSymbols.js#L15
2626
const ReactMemoType = Symbol.for("react.memo"); // https://github.com/facebook/react/blob/346c7d4c43a0717302d446da9e7423a8e28d8996/packages/shared/ReactSymbols.js#L30
27-
const ProxyInstance = new WeakMap<FunctionComponent<any>, FunctionComponent<any>>();
27+
const ProxyInstance = new WeakMap<
28+
FunctionComponent<any>,
29+
FunctionComponent<any>
30+
>();
2831
const SupportsProxy = typeof Proxy === "function";
2932

3033
const ProxyHandlers = {
@@ -229,8 +232,6 @@ export function useSignalEffect(cb: () => void | (() => void)) {
229232
callback.current = cb;
230233

231234
useEffect(() => {
232-
return effect(() => {
233-
return callback.current();
234-
});
235+
return effect(() => callback.current());
235236
}, Empty);
236237
}

‎packages/react/test/index.test.tsx‎

Lines changed: 133 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
// @ts-ignore-next-line
22
globalThis.IS_REACT_ACT_ENVIRONMENT = true;
33

4-
import { signal, useComputed } from "@preact/signals-react";
5-
import { createElement, useMemo, memo, StrictMode } from "react";
4+
import { signal, useComputed, useSignalEffect } from "@preact/signals-react";
5+
import { createElement, useMemo, memo, StrictMode, createRef } from "react";
66
import { createRoot, Root } from "react-dom/client";
77
import { renderToStaticMarkup } from "react-dom/server";
88
import { act } from "react-dom/test-utils";
@@ -222,4 +222,135 @@ describe("@preact/signals-react", () => {
222222
}
223223
});
224224
});
225+
226+
describe("useSignalEffect()", () => {
227+
it("should be invoked after commit", async () => {
228+
const ref = createRef<HTMLDivElement>();
229+
const sig = signal("foo");
230+
const spy = sinon.spy();
231+
let count = 0;
232+
233+
function App() {
234+
useSignalEffect(() =>
235+
spy(
236+
sig.value,
237+
ref.current,
238+
ref.current!.getAttribute("data-render-id")
239+
)
240+
);
241+
return (
242+
<p ref={ref} data-render-id={count++}>
243+
{sig.value}
244+
</p>
245+
);
246+
}
247+
248+
render(<App />);
249+
expect(scratch.textContent).to.equal("foo");
250+
251+
expect(spy).to.have.been.calledOnceWith(
252+
"foo",
253+
scratch.firstElementChild,
254+
"0"
255+
);
256+
257+
spy.resetHistory();
258+
259+
act(() => {
260+
sig.value = "bar";
261+
});
262+
263+
expect(scratch.textContent).to.equal("bar");
264+
265+
// NOTE: Ideally, call should receive "1" as its third argument!
266+
// The "0" indicates that Preact's DOM mutations hadn't yet been performed when the callback ran.
267+
// This happens because we do signal-based effect runs after the first, not VDOM.
268+
// Perhaps we could find a way to defer the callback when it coincides with a render?
269+
expect(spy).to.have.been.calledOnceWith(
270+
"bar",
271+
scratch.firstElementChild,
272+
"0" // ideally "1" - update if we find a nice way to do so!
273+
);
274+
});
275+
276+
it("should invoke any returned cleanup function for updates", async () => {
277+
const ref = createRef<HTMLDivElement>();
278+
const sig = signal("foo");
279+
const spy = sinon.spy();
280+
const cleanup = sinon.spy();
281+
let count = 0;
282+
283+
function App() {
284+
useSignalEffect(() => {
285+
const id = ref.current!.getAttribute("data-render-id");
286+
const value = sig.value;
287+
spy(value, ref.current, id);
288+
return () => cleanup(value, ref.current, id);
289+
});
290+
return (
291+
<p ref={ref} data-render-id={count++}>
292+
{sig.value}
293+
</p>
294+
);
295+
}
296+
297+
render(<App />);
298+
299+
expect(cleanup).not.to.have.been.called;
300+
expect(spy).to.have.been.calledOnceWith(
301+
"foo",
302+
scratch.firstElementChild,
303+
"0"
304+
);
305+
spy.resetHistory();
306+
307+
act(() => {
308+
sig.value = "bar";
309+
});
310+
311+
expect(scratch.textContent).to.equal("bar");
312+
313+
const child = scratch.firstElementChild;
314+
315+
expect(cleanup).to.have.been.calledOnceWith("foo", child, "0");
316+
317+
expect(spy).to.have.been.calledOnceWith(
318+
"bar",
319+
child,
320+
"0" // ideally "1" - update if we find a nice way to do so!
321+
);
322+
});
323+
324+
it("should invoke any returned cleanup function for unmounts", async () => {
325+
const ref = createRef<HTMLDivElement>();
326+
const sig = signal("foo");
327+
const spy = sinon.spy();
328+
const cleanup = sinon.spy();
329+
330+
function App() {
331+
useSignalEffect(() => {
332+
const value = sig.value;
333+
spy(value, ref.current);
334+
return () => cleanup(value, ref.current);
335+
});
336+
return <p ref={ref}>{sig.value}</p>;
337+
}
338+
339+
render(<App />);
340+
341+
const child = scratch.firstElementChild;
342+
343+
expect(cleanup).not.to.have.been.called;
344+
expect(spy).to.have.been.calledOnceWith("foo", child);
345+
spy.resetHistory();
346+
347+
render(null);
348+
349+
expect(spy).not.to.have.been.called;
350+
expect(cleanup).to.have.been.calledOnce;
351+
// @note: React cleans up the ref eagerly, so it's already null by the time the callback runs.
352+
// this is probably worth fixing at some point.
353+
expect(cleanup).to.have.been.calledWith("foo", null);
354+
});
355+
});
225356
});

0 commit comments

Comments
 (0)