diff --git a/.changeset/error-boundary-superseded-staged-error-4527.md b/.changeset/error-boundary-superseded-staged-error-4527.md new file mode 100644 index 000000000..467d1657a --- /dev/null +++ b/.changeset/error-boundary-superseded-staged-error-4527.md @@ -0,0 +1,7 @@ +--- +"@solidjs/signals": patch +--- + +An error boundary no longer hands its reader an error that was superseded inside the same transition (#4527). Here is the case: an action keeps its transition open, the store's re-ask is rejected (that rejection is staged, D-41), and a second re-ask answers before the commit. An effect whose compute created `createErrorBoundary(() => store, …)` used to get the fallback at the commit and never ran again. Now it gets the answer, as the screen does. + +Two changes make that work. First, a pass that a derived store's pull throws a staged error into is now linked to the store's derive, including the untracked shape probe of a returned proxy. That way the re-ask and the answer that supersede the error reach the memo. Second, an error boundary no longer waits for first content: one that has not shown content reveals a recovery that lands in the hold it caught the error in together with that hold's commit, instead of keeping its fallback past it. diff --git a/packages/signals/src/boundaries.ts b/packages/signals/src/boundaries.ts index b575ba47e..e679357f4 100644 --- a/packages/signals/src/boundaries.ts +++ b/packages/signals/src/boundaries.ts @@ -363,7 +363,11 @@ export function prune(b: Boundary, pass: boolean): number { // lane's reader reveals with the lane, this round.) A boundary that // has not shown content catches it instead: the fallback stays until // the reader is committed (#3540: what a fresh boundary catches holds - // nothing outside it). + // nothing outside it). Not an error boundary (born `_initialized`): + // what it caught in a hold was that hold's staged error (D-41), and an + // answer landing in the same hold replaces it there — the content + // reveals with the commit, and nothing ever shows the superseded + // error (D-39, #4527). if (!pass || !b._initialized) continue; joinFuture(txOf(s)); } @@ -612,7 +616,8 @@ function createBoundary( _tree: null!, _output: null, _readers: new Map(), - _initialized: false, + // Loading only: an error boundary has no first-content wait (`prune`). + _initialized: type === STATUS_ERROR, _armed: false, _armLane: null, _lane: null, diff --git a/packages/signals/src/store/store.ts b/packages/signals/src/store/store.ts index f4b80c71d..657ee9891 100644 --- a/packages/signals/src/store/store.ts +++ b/packages/signals/src/store/store.ts @@ -1885,6 +1885,11 @@ function pullFamily(target: StoreTarget): void { // flight the family has — not over a new question they do not answer // (optimistic.ts `stands`, #4336). if (verdict === null && fw._statusFlags & STATUS_PENDING && optHooks?.stands(target)) return; + // A pass the pull throws a staged error (D-41) is linked to the derive, + // untracked too — `handleAsync` asking a returned proxy whether it is a + // flight: the pass carries the error, and the re-ask or answer that + // supersedes it must reach it (#4527; a tracked read links anyway, #4519). + if (c && fw._config & CONFIG_STAGED_ERROR) link(fw, c); readNode(fw); } else { untrack(() => readNode(fw)); diff --git a/packages/signals/tests/error-boundary-superseded-staged-error-4527.test.ts b/packages/signals/tests/error-boundary-superseded-staged-error-4527.test.ts new file mode 100644 index 000000000..7583c4294 --- /dev/null +++ b/packages/signals/tests/error-boundary-superseded-staged-error-4527.test.ts @@ -0,0 +1,130 @@ +/** + * #4527: an action re-asks a derived store and a memo, so its transition + * stays open. The store's re-ask rejects (staged, D-41), then a second + * re-ask answers before the transition commits. An effect whose compute + * creates an error boundary over the bare store proxy got the superseded + * error at the commit (the boundary's fallback, `null`) and never ran again. + * + * Two holes, both D-41 / D-39: the memo returning the proxy was thrown the + * staged error by `handleAsync`'s shape probe (an untracked read that pulls + * the derive) without a link to the derive, so neither the second re-ask nor + * its answer reached it; and an error boundary that never showed content + * kept its fallback over a reader whose replacement had landed in the same + * hold, so the commit showed an error nothing on screen carried. + */ +import { expect, it } from "vitest"; +import { + action, + createEffect, + createErrorBoundary, + createLoadingBoundary, + createMemo, + createOptimisticStore, + createRenderEffect, + createRoot, + createStore, + flush, + refresh, + untrack +} from "../src/index.js"; + +const tick = async () => { + for (let i = 0; i < 4; i++) { + await new Promise(r => setTimeout(r, 0)); + flush(); + } +}; + +for (const kind of ["optimistic", "store"] as const) + for (const effect of ["effect", "render"] as const) + it(`#4527 ${kind} ${effect}: the commit shows the answer, not the superseded error`, async () => { + const asks: { key: string; res: (v: any) => void; rej: (e: any) => void }[] = []; + const ask = (key: string) => new Promise((res, rej) => asks.push({ key, res, rej })); + const settle = async (key: string, f: (a: (typeof asks)[number]) => void) => { + f( + asks.splice( + asks.findIndex(a => a.key === key), + 1 + )[0] + ); + await tick(); + }; + const seen: string[] = []; + const frames: string[] = []; + let screen = ""; + const snap = (label: string) => { + frames.push(`${label}: ${screen} | ${seen.join(",")}`); + seen.length = 0; + }; + let again!: () => void; + let rename!: (t: string) => any; + const dispose = createRoot(d => { + const [issues] = + kind === "store" + ? createStore(() => ask("issues"), []) + : createOptimisticStore(() => ask("issues"), [], { key: "id" }); + again = () => refresh(issues); + const detail = createMemo(() => ask("detail")); + rename = action(function* (t: string) { + yield ask(`rename:${t}`); + refresh(issues); + refresh(detail); + }); + const compute = () => + untrack(() => + createErrorBoundary( + () => issues, + () => null + ) + )(); + const out = (v: any) => void seen.push(v === null ? "null" : `len${v.length}`); + if (effect === "render") createRenderEffect(compute, out); + else + createEffect(compute, { + effect: out, + error: (e: any) => void seen.push(`error ${e?.message}`) + } as any); + const list = createLoadingBoundary( + createErrorBoundary( + () => issues.map((r: any) => r.title).join(","), + (e: any) => `err ${e().message}` + ), + () => "loading" + ); + const info = createLoadingBoundary( + () => String(detail()), + () => "…" + ); + createRenderEffect( + () => `${list()} / ${info()}`, + v => void (screen = v) + ); + return d; + }); + await tick(); + const rows = (t: string) => [ + { id: 1, title: "Login" }, + { id: 3, title: t }, + { id: 4, title: "Typo" } + ]; + await settle("issues", a => a.res(rows("Crash"))); + await settle("detail", a => a.res("d1")); + seen.length = 0; + rename("Alpha"); + await tick(); + await settle("rename:Alpha", a => a.res({})); + await settle("issues", a => a.rej(new Error("declined"))); + snap("re-ask failed"); + again(); + await tick(); + await settle("issues", a => a.res(rows("Alpha").filter(r => r.id !== 4))); + await settle("detail", a => a.res("d2")); + snap("committed"); + expect(frames).toEqual([ + // D-41: the rejection is staged with the held frame — nothing shows it. + "re-ask failed: Login,Crash,Typo / d1 | ", + // The second answer superseded it before the commit: nothing sees it. + "committed: Login,Alpha / d2 | len2" + ]); + dispose(); + }); diff --git a/packages/web/test/error-boundary-superseded-error-4527.spec.tsx b/packages/web/test/error-boundary-superseded-error-4527.spec.tsx new file mode 100644 index 000000000..38cbef257 --- /dev/null +++ b/packages/web/test/error-boundary-superseded-error-4527.spec.tsx @@ -0,0 +1,124 @@ +/** + * @jsxImportSource @solidjs/web + * @vitest-environment jsdom + */ +// #4527: an action re-asks a store and a memo, so its transition stays open. +// The store's re-ask fails (staged, D-41), then a second re-ask answers +// before the transition commits. An effect reading the store through an +// error boundary created in its compute got the superseded failure (the +// fallback's `null`) at the commit and never ran again; the screen showed +// the new rows. Nothing may see the superseded failure: the effect gets +// `len2` with the screen. +import { expect, test } from "vitest"; +import { + For, + Loading, + Errored, + action, + createOptimisticStore, + createMemo, + createEffect, + untrack, + flush, + refresh +} from "solid-js"; +import { createErrorBoundary } from "solid-js/internal"; +import { render } from "../src/index.js"; + +const tick = async () => { + for (let i = 0; i < 4; i++) { + await new Promise(r => setTimeout(r, 0)); + flush(); + } +}; + +test("#4527: an effect's error boundary gets the answer, not the superseded error", async () => { + const asks: { key: string; res: (v: any) => void; rej: (e: any) => void }[] = []; + const ask = (key: string) => new Promise((res, rej) => asks.push({ key, res, rej })); + const settle = async (key: string, f: (a: (typeof asks)[number]) => void) => { + f( + asks.splice( + asks.findIndex(a => a.key === key), + 1 + )[0] + ); + await tick(); + }; + const container = document.body.appendChild(document.createElement("div")); + const log: string[] = []; + const frames: string[] = []; + const snap = (label: string) => { + frames.push(`${label}: ${container.textContent} | ${log.join(",")}`); + log.length = 0; + }; + let again!: () => void; + let rename!: (t: string) => any; + const dispose = render(() => { + const [issues] = createOptimisticStore(() => ask("issues"), [], { key: "id" }); + again = () => refresh(issues); + const detail = createMemo(() => ask("detail")); + rename = action(function* (t: string) { + yield ask(`rename:${t}`); + refresh(issues); + refresh(detail); + }); + createEffect( + () => + untrack(() => + createErrorBoundary( + () => issues, + () => null + ) + )(), + { + effect: (v: any) => void log.push(v === null ? "null" : `len${v.length}`), + error: (e: any) => void log.push(`error ${e?.message}`) + } as any + ); + return ( + <> + + `list error ${e().message}`}> + + {(it: any) => ( + + [#{it.id} {it.title}] + + )} + + + + {" | "} + {String(detail())} + + ); + }, container); + await tick(); + const rows = (t: string) => [ + { id: 1, title: "Login" }, + { id: 3, title: t }, + { id: 4, title: "Typo" } + ]; + await settle("issues", a => a.res(rows("Crash"))); + await settle("detail", a => a.res("d1")); + snap("loaded"); + rename("Alpha"); + await tick(); + await settle("rename:Alpha", a => a.res({})); + await settle("issues", a => a.rej(new Error("declined"))); + snap("re-ask failed"); + again(); + await tick(); + await settle("issues", a => a.res(rows("Alpha").filter(r => r.id !== 4))); + snap("re-ask answered"); + await settle("detail", a => a.res("d2")); + snap("detail answered"); + expect(frames).toEqual([ + "loaded: [#1 Login][#3 Crash][#4 Typo] | d1 | len3", + "re-ask failed: [#1 Login][#3 Crash][#4 Typo] | d1 | ", + "re-ask answered: [#1 Login][#3 Crash][#4 Typo] | d1 | ", + "detail answered: [#1 Login][#3 Alpha] | d2 | len2" + ]); + dispose(); + container.remove(); +});