diff --git a/.changeset/cleanup-write-during-teardown.md b/.changeset/cleanup-write-during-teardown.md new file mode 100644 index 000000000..af4c873d6 --- /dev/null +++ b/.changeset/cleanup-write-during-teardown.md @@ -0,0 +1,5 @@ +--- +"@solidjs/signals": patch +--- + +Allow writes in cleanups during teardown (#4165). Removing a keyed `` row ran the row's cleanups inside the list's pass, so a setter in an `onCleanup` or `onSettled` cleanup was rejected in development as a write in an owned scope, which halted reactivity. A cleanup is teardown, not part of the computation disposing it, so the dev-only owned-scope write check now stands down while cleanups run. Cleanups keep their owner and context, and production builds are unchanged. diff --git a/packages/signals/src/core/core.ts b/packages/signals/src/core/core.ts index a49dc13c5..18a53fef4 100644 --- a/packages/signals/src/core/core.ts +++ b/packages/signals/src/core/core.ts @@ -1489,7 +1489,11 @@ export function exitCallback(): void { } /** * Dev-only: > 0 while owner teardown runs cleanups (`_disposal` entries and - * effect-returned cleanups — owner.ts), for the same reason. Kept apart from + * effect-returned cleanups — owner.ts), for the same reason. The owned-scope + * write guard (setSignal, markRefresh, the store setter) stands down here + * too: a cleanup is teardown, not part of whichever computation is disposing + * its owner (a list pass removing a row, a branch switching away), so its + * writes are imperative like an effect callback's (#4165). Kept apart from * `callbackDepth` because its sites cannot use try/finally (the frame would * survive into prod): a throwing cleanup leaves it raised, and dev.ts resets * it on the next microtask, which teardown — synchronous — never spans. @@ -2106,7 +2110,8 @@ export function setSignal(el: Signal | Computed, v: T | ((prev: T) => T __DEV__ && !(el._config & CONFIG_OWNED_WRITE) && !(context && context._config & CONFIG_CHILDREN_FORBIDDEN) && - context + context && + !disposalDepth ) { emitDiagnostic({ code: "REACTIVE_WRITE_IN_OWNED_SCOPE", @@ -2304,7 +2309,8 @@ export function markRefresh(node: Computed): void { __DEV__ && context && !((node._config ?? 0) & CONFIG_OWNED_WRITE) && - !(context._config & CONFIG_CHILDREN_FORBIDDEN) + !(context._config & CONFIG_CHILDREN_FORBIDDEN) && + !disposalDepth ) { emitDiagnostic({ code: "REACTIVE_WRITE_IN_OWNED_SCOPE", diff --git a/packages/signals/src/store/store.ts b/packages/signals/src/store/store.ts index c91b72546..ddb53c1fb 100644 --- a/packages/signals/src/store/store.ts +++ b/packages/signals/src/store/store.ts @@ -49,6 +49,7 @@ import { import { captureWriteSnapshot, context, + disposalDepth, isEqual, notePromotedWrite, ownedScopeWriteMessage, @@ -2293,7 +2294,7 @@ const ASYNC_STORE_SETTER_MESSAGE = * write violation — the user guard (`mapArray`'s pattern: the node-level * writes carry `ownedWrite`; the setter asks). */ function devGuardStoreSetterWrite(): void { - if (context && !(context._config & CONFIG_CHILDREN_FORBIDDEN)) { + if (context && !(context._config & CONFIG_CHILDREN_FORBIDDEN) && !disposalDepth) { emitDiagnostic({ code: "REACTIVE_WRITE_IN_OWNED_SCOPE", kind: "write", diff --git a/packages/signals/tests/for-row-cleanup-write-4165.test.ts b/packages/signals/tests/for-row-cleanup-write-4165.test.ts new file mode 100644 index 000000000..603e77005 --- /dev/null +++ b/packages/signals/tests/for-row-cleanup-write-4165.test.ts @@ -0,0 +1,100 @@ +/** + * A cleanup is teardown, so a setter in it is not a write in the owned scope + * of whichever computation is tearing the owner down: a list pass removing a + * row, or a branch memo switching away (#4165). The cleanup still runs under + * its owner, so `getOwner` and context lookups keep working. + * + * @see https://github.com/solidjs/solid/issues/4165 + */ +import { expect, it } from "vitest"; +import { + createContext, + createMemo, + createRenderEffect, + createRoot, + createSignal, + createStore, + flush, + getContext, + getOwner, + mapArray, + onCleanup, + setContext, + untrack +} from "../src/index.js"; + +it("a removed row's cleanup can write a signal", () => { + const [rows, setRows] = createSignal([1, 2]); + const [count, setCount] = createSignal(0); + const seen: unknown[] = []; + createRoot(() => { + const mapped = mapArray(rows, r => { + onCleanup(() => setCount(n => n + 1)); + return r; + }); + createRenderEffect( + () => mapped(), + v => void seen.push([...v]) + ); + }); + flush(); + setRows([2]); + flush(); + setRows([2, 3]); + flush(); + expect(seen).toEqual([[1, 2], [2], [2, 3]]); + expect(count()).toBe(1); +}); + +it("a branch's cleanup can write a signal when the branch switches away", () => { + const [on, setOn] = createSignal(true); + const [count, setCount] = createSignal(0); + createRoot(() => { + const branch = createMemo(() => { + if (!on()) return null; + return untrack(() => { + onCleanup(() => setCount(n => n + 1)); + return 1; + }); + }); + createRenderEffect(branch, () => {}); + }); + flush(); + setOn(false); + flush(); + expect(count()).toBe(1); +}); + +it("a removed row's cleanup can write a store", () => { + const [rows, setRows] = createSignal([1, 2]); + const [state, setState] = createStore({ removed: [] as number[] }); + createRoot(() => { + const mapped = mapArray(rows, r => { + onCleanup(() => setState(s => void s.removed.push(r))); + return r; + }); + createRenderEffect(mapped, () => {}); + }); + flush(); + setRows([2]); + flush(); + expect(state.removed).toEqual([1]); +}); + +it("a removed row's cleanup still sees its owner and context", () => { + const Ctx = createContext("default"); + const [rows, setRows] = createSignal([1, 2]); + const seen: unknown[] = []; + createRoot(() => { + setContext(Ctx, "provided"); + const mapped = mapArray(rows, r => { + onCleanup(() => seen.push([r, !!getOwner(), getContext(Ctx)])); + return r; + }); + createRenderEffect(mapped, () => {}); + }); + flush(); + setRows([2]); + flush(); + expect(seen).toEqual([[1, true, "provided"]]); +}); diff --git a/packages/web/test/for-row-cleanup-write-4165.spec.tsx b/packages/web/test/for-row-cleanup-write-4165.spec.tsx new file mode 100644 index 000000000..b18763a5f --- /dev/null +++ b/packages/web/test/for-row-cleanup-write-4165.spec.tsx @@ -0,0 +1,37 @@ +/** + * @jsxImportSource @solidjs/web + * @vitest-environment jsdom + */ +// #4165: removing a keyed row ran the row's cleanups inside the list's +// pass, so a setter in a cleanup was rejected as a write in an owned scope and +// halted reactivity in development. Cleanups now run with no owner. +import { afterEach, expect, test } from "vitest"; +import { createSignal, flush, For, onCleanup, onSettled, resetErrorHalt } from "solid-js"; +import { render } from "../src/index.js"; + +afterEach(() => resetErrorHalt()); + +for (const [label, register] of [ + ["onSettled cleanup", (fn: () => void) => onSettled(() => fn)], + ["onCleanup", (fn: () => void) => onCleanup(fn)] +] as const) + test(`a row's ${label} can write a signal when the row is removed`, () => { + const [rows, setRows] = createSignal([1, 2]); + const [count, setCount] = createSignal(0); + function Row(props: { n: number }) { + register(() => setCount(n => n + 1)); + return {props.n}; + } + const div = document.createElement("div"); + const dispose = render(() => {n => }, div); + flush(); + expect(div.textContent).toBe("12"); + setRows([2]); + flush(); + expect(div.textContent).toBe("2"); + expect(count()).toBe(1); + setRows([2, 3]); + flush(); + expect(div.textContent).toBe("23"); + dispose(); + });