diff --git a/.changeset/restaged-effect-no-early-run.md b/.changeset/restaged-effect-no-early-run.md new file mode 100644 index 000000000..058c7fd7a --- /dev/null +++ b/.changeset/restaged-effect-no-early-run.md @@ -0,0 +1,5 @@ +--- +"@solidjs/signals": patch +--- + +A render effect created while an action holds a value it reads no longer runs its callback when a later pass re-stages it before the action lands (#3802). That run passed `undefined` as the value, because the effect had nothing committed yet, so compiled JSX bindings threw and halted reactivity. The effect now runs once, at the commit that lands the action, with the latest staged value. diff --git a/packages/signals/src/core/core.ts b/packages/signals/src/core/core.ts index 80f163985..2ee50e388 100644 --- a/packages/signals/src/core/core.ts +++ b/packages/signals/src/core/core.ts @@ -570,7 +570,9 @@ export function recompute(el: Computed, create: boolean = false): void { el._flags & REACTIVE_SCREEN_READ && !(el as any)._modified && Object.is((el as any)._prevValue, value) - ) + ) && + // A re-staging pass owes no run: the commit replays the effect (#3802). + el._pendingValue === NOT_PENDING ) { (el as any)._modified = !el._x?._error; // Reuse one bound runner per effect — runEffect no-ops on a stale diff --git a/packages/signals/tests/held-effect-restage-run-3802.test.ts b/packages/signals/tests/held-effect-restage-run-3802.test.ts new file mode 100644 index 000000000..98ed7ad0d --- /dev/null +++ b/packages/signals/tests/held-effect-restage-run-3802.test.ts @@ -0,0 +1,122 @@ +// #3802: a render effect born held under an action, re-run by a write to a +// committed source, ran its callback with no committed value. + +import { afterEach, expect, it, vi } from "vitest"; +import { + action, + createMemo, + createOptimistic, + createRenderEffect, + createRoot, + createSignal, + flush +} from "../src/index.js"; + +afterEach(() => vi.restoreAllMocks()); + +async function settle() { + for (let i = 0; i < 8; i++) await Promise.resolve(); + flush(); +} + +it("a re-staging pass of a born-held effect does not run it before the commit", async () => { + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + const [value, setValue] = createSignal(false); + const [mounted, setMounted] = createSignal(false); + const [title, setTitle] = createSignal(""); + let resolve!: () => void; + const gate = new Promise(r => (resolve = r)); + const runs: [boolean, string][] = []; + + const save = action(function* () { + setValue(true); + yield gate; + }); + + createRoot(() => { + const child = createMemo(() => { + if (!mounted()) return; + createMemo(() => value()); + createRenderEffect( + () => ({ v: value(), t: title() }), + state => { + runs.push([state.v, state.t]); + } + ); + }); + createRenderEffect(child, () => {}); + }); + flush(); + + const pending = save(); + await settle(); + setMounted(true); + await settle(); + setTitle("Updated"); + await settle(); + + expect(error).not.toHaveBeenCalled(); + expect(runs).toEqual([]); + + resolve(); + await pending; + await settle(); + + expect(error).not.toHaveBeenCalled(); + expect(runs).toEqual([[true, "Updated"]]); +}); + +// The same effect re-derived by an optimistic write's lane rather than a +// mainline write: the lane's run had nothing committed to apply either. +it("a lane pass of a born-held effect does not run it with no committed value", async () => { + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + const [value, setValue] = createSignal(false); + const [mounted, setMounted] = createSignal(false); + const [guess, setGuess] = createOptimistic(0); + let resolveSave!: () => void; + let resolveVote!: () => void; + const runs: [boolean, number][] = []; + + const save = action(function* () { + setValue(true); + yield new Promise(r => (resolveSave = r)); + }); + const vote = action(function* () { + setGuess(5); + yield new Promise(r => (resolveVote = r)); + }); + + createRoot(() => { + const child = createMemo(() => { + if (!mounted()) return; + createMemo(() => value()); + createRenderEffect( + () => ({ v: value(), g: guess() }), + state => { + runs.push([state.v, state.g]); + } + ); + }); + createRenderEffect(child, () => {}); + }); + flush(); + + const saving = save(); + await settle(); + setMounted(true); + await settle(); + const voting = vote(); + await settle(); + + expect(error).not.toHaveBeenCalled(); + + resolveVote(); + await voting; + await settle(); + resolveSave(); + await saving; + await settle(); + + expect(error).not.toHaveBeenCalled(); + expect(runs.at(-1)).toEqual([true, 0]); +}); diff --git a/packages/web/test/held-action-memo-mount-3802.spec.tsx b/packages/web/test/held-action-memo-mount-3802.spec.tsx new file mode 100644 index 000000000..378fe3cb6 --- /dev/null +++ b/packages/web/test/held-action-memo-mount-3802.spec.tsx @@ -0,0 +1,68 @@ +/** + * @jsxImportSource @solidjs/web + * @vitest-environment jsdom + */ +import { afterEach, expect, test, vi } from "vitest"; +import { Show, action, createMemo, createSignal, flush } from "solid-js"; +import { render } from "../src/index.js"; + +function deferred() { + let resolve!: () => void; + const promise = new Promise(r => (resolve = r)); + return { promise, resolve }; +} + +async function settle() { + for (let i = 0; i < 8; i++) await Promise.resolve(); + flush(); +} + +afterEach(() => vi.restoreAllMocks()); + +test("updating an attribute of a subtree mounted under a held action does not crash (#3802)", async () => { + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + const div = document.createElement("div"); + const [value, setValue] = createSignal(false); + const [mounted, setMounted] = createSignal(false); + const [title, setTitle] = createSignal(""); + const gate = deferred(); + + const save = action(function* () { + setValue(true); + yield gate.promise; + }); + + const dispose = render( + () => ( +
+ + {_ => { + createMemo(() => value()); + return
; + }} + +
+ ), + div + ); + flush(); + + const pending = save(); + await settle(); + setMounted(true); + await settle(); + setTitle("Updated"); + await settle(); + + expect(error).not.toHaveBeenCalled(); + + gate.resolve(); + await pending; + await settle(); + + expect(error).not.toHaveBeenCalled(); + const el = div.querySelector("div")!; + expect(el.className).toBe("true"); + expect(el.title).toBe("Updated"); + dispose(); +});