diff --git a/.changeset/errored-memo-over-output-held-reject.md b/.changeset/errored-memo-over-output-held-reject.md new file mode 100644 index 000000000..0157c8302 --- /dev/null +++ b/.changeset/errored-memo-over-output-held-reject.md @@ -0,0 +1,5 @@ +--- +"@solidjs/signals": patch +--- + +A memo over an error boundary's output no longer halts when the boundary's source rejects during a held change: the boundary shows its fallback, as it does when a render effect reads the output directly (#4640). diff --git a/packages/signals/src/core/verdict.ts b/packages/signals/src/core/verdict.ts index ac78ca8d4..e56df13fc 100644 --- a/packages/signals/src/core/verdict.ts +++ b/packages/signals/src/core/verdict.ts @@ -722,18 +722,26 @@ function holdWork(n: Computed, source: Computed): unknown { function stageMark(node: Computed, el: Computed): void { if ( el._config & CONFIG_STAGED_ERROR && - !( - node._config & - (CONFIG_REDERIVE | CONFIG_STAGED_ERROR | CONFIG_ASYNC | (EFFECT_TRACKED << 23)) - ) + !(node._config & (CONFIG_REDERIVE | CONFIG_STAGED_ERROR | (EFFECT_TRACKED << 23))) ) { - node._pendingValue = el._pendingValue; - node._config |= CONFIG_STAGED_ERROR; - queuePendingNode(node); - // A derived store's readers are its family's (store/projection.ts): they - // re-derive from the staging, as from the derive's status (#4189). - (node as any)._fam?.st(STATUS_ERROR); - settlePendingSource(node); + // Only a node that reads a staging carries it. The walk also reaches the + // readers of a node it does not mark (a boundary's output, whose pass + // decides content or fallback): what they get comes through that node, + // not as the staging. An async memo over a staging is marked, so its + // readers stay pending until the commit (F-9, D-22, D-23). + // A memo over an error boundary's output is a plain reader of it (F-3: + // where the memo sits does not matter); marked, it threw the error past + // the boundary that caught it, and the commit halted (#4640). + for (let d = node._deps; d; d = d._nextDep) + if (d._dep._config & CONFIG_STAGED_ERROR) { + node._pendingValue = el._pendingValue; + node._config |= CONFIG_STAGED_ERROR; + queuePendingNode(node); + // A derived store's readers are its family's (store/projection.ts): they + // re-derive from the staging, as from the derive's status (#4189). + (node as any)._fam?.st(STATUS_ERROR); + return settlePendingSource(node); + } } } @@ -744,7 +752,10 @@ function stageMark(node: Computed, el: Computed): void { function keepReaders(el: Computed): void { for (let s = el._subs; s !== null; s = s._nextSub) { const r = s._sub; - if ((r._config & (CONFIG_HELD | CONFIG_STAGED)) !== (CONFIG_HELD | CONFIG_STAGED) || (r as any)._type) + if ( + (r._config & (CONFIG_HELD | CONFIG_STAGED)) !== (CONFIG_HELD | CONFIG_STAGED) || + (r as any)._type + ) continue; let d = r._deps; while (d !== null && (d._dep === el || !(d._dep._config & CONFIG_HELD))) d = d._nextDep; diff --git a/packages/signals/tests/errored-memo-over-output-held-reject-4640.test.ts b/packages/signals/tests/errored-memo-over-output-held-reject-4640.test.ts new file mode 100644 index 000000000..5e5b286e0 --- /dev/null +++ b/packages/signals/tests/errored-memo-over-output-held-reject-4640.test.ts @@ -0,0 +1,97 @@ +/** + * #4640: an error boundary built outside (`untrack`) over an async memo; + * the boundary's output is read by a render effect, directly or through a + * memo, in the open or under a `` without `on`. The first ask + * answers "A"; a write starts a held change; the second ask rejects. + * + * The rejection is the boundary's: it shows its fallback, and nothing halts + * (F-12, D-39). A memo is not a reader in its own right — it counts only + * through the render effect that reads it, wherever it was created + * (Vocabulary "Reader"; F-3: "where the memo was created does not matter"), + * so every shape shows the same frames. The memo over the output read the + * output, not the staged error (D-41: only "a derivation that reads a staged + * error throws it"), yet the settle walk from the rejected source marked it + * with the staging through the output it does not mark; the commit then + * published the error on the memo, past the boundary, and halted. + */ +import { expect, it } from "vitest"; +import { + createErrorBoundary, + createLoadingBoundary, + createMemo, + createRenderEffect, + createRoot, + createSignal, + flush, + untrack +} from "../src/index.js"; + +const tick = async () => { + for (let k = 0; k < 4; k++) { + await new Promise(r => setTimeout(r, 0)); + flush(); + } +}; + +it.each([ + ["direct", "open"], + ["memo", "open"], + ["direct", "loading"], + ["memo", "loading"] +] as const)( + "a held rejection under an error boundary built outside shows its fallback (%s, %s)", + async (between, place) => { + const asks: { res: (v: string) => void; rej: (e: unknown) => void }[] = []; + const frames: unknown[] = []; + const [sel, setSel] = createSignal(3); + const dispose = createRoot(d => { + const detail = createMemo(() => { + sel(); + return new Promise((res, rej) => asks.push({ res, rej })); + }); + const shown = untrack(() => + createErrorBoundary( + () => detail(), + () => "ERR" + ) + ); + const t = between === "memo" ? createMemo(() => shown()) : shown; + const view = + place === "loading" + ? untrack(() => + createLoadingBoundary( + () => t(), + () => "fb" + ) + ) + : t; + createRenderEffect( + () => view(), + v => void frames.push(v) + ); + return d; + }); + // The first load: the root attach waits for it in the open (D-6); a + // fresh shows its fallback (F-4). + const first = place === "loading" ? ["fb", "A"] : ["A"]; + try { + await tick(); + asks.shift()!.res("A"); + await tick(); + expect(frames).toEqual(first); + + setSel(1); + flush(); + await tick(); + // Held: the screen keeps what it showed. + expect(frames).toEqual(first); + + asks.shift()!.rej(new Error("boom")); + await tick(); + // The boundary's fallback, no halt (an uncaught error fails the run). + expect(frames).toEqual([...first, "ERR"]); + } finally { + dispose(); + } + } +); diff --git a/packages/signals/tests/staged-error-through-async-memo-4640.test.ts b/packages/signals/tests/staged-error-through-async-memo-4640.test.ts new file mode 100644 index 000000000..dab058dec --- /dev/null +++ b/packages/signals/tests/staged-error-through-async-memo-4640.test.ts @@ -0,0 +1,130 @@ +/** + * #4640 review: a plain memo `m` over an async memo `am` over an async source + * `s`. A write (or an action's write) re-asks `s` and a sibling flight on the + * same write keeps the change held after `s` rejects. + * + * `m` reads the staging through `am` (D-41: "a derivation that reads a + * staged error throws it"), so its verdict stays pending until the commit + * (F-9, D-22, D-23) and an untracked `latest(m)` in that window throws the + * staged error rather than returning the old value that the commit then + * replaces (#4456). The commit shows the error once: an effect's error arm + * over `m` fires a single time. + * + * Narrowing the staging to readers of a staging (#4640) must not drop the + * async memo from the marking: unmarked, `m` reported final and `latest(m)` + * returned "A!" in the window. + */ +import { expect, it } from "vitest"; +import { + action, + createEffect, + createErrorBoundary, + createMemo, + createRenderEffect, + createRoot, + createSignal, + flush, + isPending, + latest +} from "../src/index.js"; + +const tick = async () => { + for (let k = 0; k < 4; k++) { + await new Promise(r => setTimeout(r, 0)); + flush(); + } +}; + +it.each(["write", "action"] as const)( + "a memo over an async memo over a rejected source stays pending until the commit (%s)", + async how => { + const asks: { res: (v: string) => void; rej: (e: unknown) => void }[] = []; + const keeps: ((v: string) => void)[] = []; + const frames: string[] = []; + const errors: string[] = []; + const scr = { m: "-", p: "-" }; + const [sel, setSel] = createSignal(1); + let m!: () => string; + createRoot(() => { + const s = createMemo(() => { + sel(); + return new Promise((res, rej) => asks.push({ res, rej })); + }); + const am = createMemo(async () => { + const v = s(); + await 0; + return v; + }); + m = createMemo(() => am() + "!"); + const keep = createMemo(() => { + sel(); + return new Promise(res => keeps.push(res)); + }); + const eb = (f: () => T) => createErrorBoundary(f, () => "ERR"); + const vm = eb(() => m()); + const vp = eb(() => String(isPending(m))); + const vk = eb(() => keep()); + createRenderEffect( + () => vm(), + v => void (scr.m = v) + ); + createRenderEffect( + () => vp(), + v => void (scr.p = v) + ); + createRenderEffect( + () => vk(), + () => {} + ); + createEffect(m, { + effect: () => {}, + error: e => void errors.push((e as Error).message) + }); + }); + const frame = (l: string) => frames.push(`${l}[${scr.m} pending=${scr.p}]`); + + await tick(); + asks.shift()!.res("A"); + keeps.shift()!("k"); + await tick(); + frame("shown"); + + let release: (() => void) | undefined; + if (how === "action") + action(function* () { + setSel(2); + yield new Promise(r => (release = r)); + })(); + else setSel(2); + flush(); + await tick(); + frame("held"); + + asks.shift()!.rej(new Error("boom")); + await tick(); + frame("rejected"); + let mid: string; + try { + mid = latest(m); + } catch (e) { + mid = "throws " + ((e as any)?.cause?.message ?? (e as Error).message); + } + + keeps.shift()!("k2"); + await tick(); + if (release) { + release(); + await tick(); + } + frame("committed"); + + expect(frames).toEqual([ + "shown[A! pending=false]", + "held[A! pending=true]", + "rejected[A! pending=true]", + "committed[ERR pending=false]" + ]); + expect(mid).toBe("throws boom"); + expect(errors).toEqual(["boom"]); + } +);