diff --git a/.changeset/fix-conditional-async-reveal-pending.md b/.changeset/fix-conditional-async-reveal-pending.md new file mode 100644 index 000000000..888f73187 --- /dev/null +++ b/.changeset/fix-conditional-async-reveal-pending.md @@ -0,0 +1,5 @@ +--- +"@solidjs/signals": patch +--- + +Hold conditional reveals that first observe async work started in an earlier flush. The revealing signal now stays pending until the details can commit with it, including when the reveal creates a new child reader. Preserve fresh/reset loading-boundary fallbacks and avoid opening a second transition for readers already waiting in one. diff --git a/packages/signals/src/core/scheduler.ts b/packages/signals/src/core/scheduler.ts index 84033c666..5380848d8 100644 --- a/packages/signals/src/core/scheduler.ts +++ b/packages/signals/src/core/scheduler.ts @@ -749,25 +749,36 @@ export class GlobalQueue extends Queue { // Only track async if the boundary is propagating STATUS_PENDING (not caught by boundary) if (mask & STATUS_PENDING) { if (flags & STATUS_PENDING) { - const actualError = error !== undefined ? error : node._x?._error; + // Callers pass either nothing or this node's own `_x._error`, so `??` + // is exact (a null error falls back to the same null). + const actualError = error ?? node._x?._error; // A visibility-only mark notification (the affects() boundary // channel) updates display state on its way up but must be invisible // to completion accounting BY CONSTRUCTION: it never registers a // reporter and never counts toward the loading-boundary diagnostic. if ((actualError as NotReadyError)?._markVisual) return true; - if (activeTransition && actualError) { - const source = (actualError as NotReadyError).source; - // The one sanctioned registration site (INV-3): async blockers only - // enter the transition from queue notification. - if (__DEV__) beginAsyncReporterWrites(); - let reporters = activeTransition._asyncReporters.get(source); - if (!reporters) activeTransition._asyncReporters.set(source, (reporters = new Set())); - if (__DEV__) endAsyncReporterWrites(); - const prevSize = reporters.size; - reporters.add(node); - if (reporters.size !== prevSize) { - schedule(); - GlobalQueue._wakeSuppressedProbes?.(activeTransition); + if (actualError) { + // A reveal can discover a flight started in an earlier flush. Hold + // the staged writes with that reader (A15), even if the reader is + // new. Fresh/reset loading boundaries consume pending before it + // reaches here. A reader already parked in a transition must not + // open a second one. + if (!activeTransition && !node._transition && currentBatch._pendingNodes.length) + this.initTransition(); + if (activeTransition) { + const source = (actualError as NotReadyError).source; + // The one sanctioned registration site (INV-3): async blockers only + // enter the transition from queue notification. + if (__DEV__) beginAsyncReporterWrites(); + let reporters = activeTransition._asyncReporters.get(source); + if (!reporters) activeTransition._asyncReporters.set(source, (reporters = new Set())); + if (__DEV__) endAsyncReporterWrites(); + const prevSize = reporters.size; + reporters.add(node); + if (reporters.size !== prevSize) { + schedule(); + GlobalQueue._wakeSuppressedProbes?.(activeTransition); + } } } if (__DEV__ && _enforceLoadingBoundary) _hitUnhandledAsync = true; diff --git a/packages/signals/tests/spec-async-semantics.test.ts b/packages/signals/tests/spec-async-semantics.test.ts index 394a421a2..149bfb4e0 100644 --- a/packages/signals/tests/spec-async-semantics.test.ts +++ b/packages/signals/tests/spec-async-semantics.test.ts @@ -198,6 +198,145 @@ describe("A15 (was B3): overlapping transitions settle as one unit", () => { // a *shared* reader join one transition and settle together; writes on // fully disjoint graphs keep independent transitions and settle // independently. Both halves are the spec. + it.each([ + { child: false, boundary: false }, + { child: false, boundary: true }, + { child: true, boundary: false }, + { child: true, boundary: true } + ])( + "holds a reveal of an existing async flight (child=$child, boundary=$boundary)", + async ({ child, boundary }) => { + const [value, setValue] = createSignal(0); + const [show, setShow] = createSignal(false); + const fetcher = deferredFetcher(v => v); + let valueLine: readonly [number, boolean] | undefined; + let showLine: readonly [boolean, boolean] | undefined; + let slot: number | string | undefined; + const childLog: number[] = []; + let dispose!: () => void; + + createRoot(d => { + dispose = d; + const details = createMemo(() => fetcher.fetch(value())); + createRenderEffect( + () => [value(), isPending(value)] as const, + v => { + valueLine = v; + } + ); + createRenderEffect( + () => [show(), isPending(show)] as const, + v => { + showLine = v; + } + ); + const content = () => { + if (!show()) return "hidden"; + if (!child) return details(); + createRenderEffect(details, v => { + childLog.push(v); + }); + return "shown"; + }; + const read = boundary ? createLoadingBoundary(content, () => "loading") : content; + createRenderEffect(read, v => { + slot = v; + }); + }); + + try { + flush(); + expect([valueLine, showLine, slot]).toEqual([[0, false], [false, false], "hidden"]); + + // Cover both a never-resolved source and a refetch after a previous reveal. + for (const next of [1, 2]) { + setShow(false); + flush(); + setValue(next); + flush(); + expect([valueLine, showLine, slot]).toEqual([[next, false], [false, false], "hidden"]); + const previousChildCount = childLog.length; + + // The flight started in the earlier flush, not during this reveal. + setShow(true); + flush(); + expect([valueLine, showLine, slot]).toEqual([[next, false], [false, true], "hidden"]); + expect(show()).toBe(false); + expect(isPending(value)).toBe(false); + expect(isPending(show)).toBe(true); + expect(childLog).toHaveLength(previousChildCount); + + fetcher.resolveAll(); + await settle(); + expect([valueLine, showLine, slot]).toEqual([ + [next, false], + [true, false], + child ? "shown" : next + ]); + expect(show()).toBe(true); + expect(isPending(show)).toBe(false); + if (child) expect(childLog[childLog.length - 1]).toBe(next); + } + } finally { + dispose(); + } + } + ); + + it.each(["new", "reset"])( + "lets a %s loading boundary catch an existing flight without holding the reveal", + async mode => { + const [value, setValue] = createSignal(0); + const [show, setShow] = createSignal(false); + const fetcher = deferredFetcher(v => v); + let showLine: readonly [boolean, boolean] | undefined; + let slot: number | string | undefined; + let dispose!: () => void; + + createRoot(d => { + dispose = d; + const details = createMemo(() => fetcher.fetch(value())); + createRenderEffect( + () => [show(), isPending(show)] as const, + v => { + showLine = v; + } + ); + const read = + mode === "reset" + ? createLoadingBoundary( + () => (show() ? details() : "hidden"), + () => "loading", + { on: show } + ) + : () => (show() ? createLoadingBoundary(details, () => "loading")() : "hidden"); + createRenderEffect(read, v => { + slot = v; + }); + }); + + try { + flush(); + expect([showLine, slot]).toEqual([[false, false], "hidden"]); + setValue(1); + flush(); + expect(value()).toBe(1); + + setShow(true); + flush(); + expect([showLine, slot]).toEqual([[true, false], "loading"]); + expect(show()).toBe(true); + expect(isPending(show)).toBe(false); + + fetcher.resolveAll(); + await settle(); + expect([showLine, slot]).toEqual([[true, false], 1]); + } finally { + dispose(); + } + } + ); + it("a shared reader forces one settle point: nothing commits until both asyncs resolve", async () => { const [a, setA] = createSignal(1); const [b, setB] = createSignal(1); diff --git a/packages/web/test/loading.spec.tsx b/packages/web/test/loading.spec.tsx index 0f1eb09fc..e2d4651cb 100644 --- a/packages/web/test/loading.spec.tsx +++ b/packages/web/test/loading.spec.tsx @@ -658,6 +658,55 @@ describe("Testing Loading", () => { localDispose(); }); + test("a conditional reveal waits for an async flight from an earlier flush", async () => { + const localDiv = document.createElement("div"); + let click!: () => void; + const localDispose = render(() => { + const [value, setValue] = createSignal(0); + const [show, setShow] = createSignal(false); + const details = createMemo(async () => { + const v = value(); + await new Promise(r => setTimeout(r, 3000)); + return v; + }); + click = () => { + setValue(v => v + 1); + setTimeout(() => setShow(true), 1000); + }; + return ( +
+
+ value(): {value()} {isPending(value) ? "pending" : null} +
+
+ show(): {String(show())} {isPending(show) ? "pending" : null} +
+
{show() ? details() : "hidden"}
+
+ ); + }, localDiv); + const lines = () => + Array.from(localDiv.firstElementChild!.children, el => el.textContent!.trim()); + + try { + flush(); + expect(lines()).toEqual(["value(): 0", "show(): false", "hidden"]); + click(); + flush(); + expect(lines()).toEqual(["value(): 1", "show(): false", "hidden"]); + + await vi.advanceTimersByTimeAsync(1000); + flush(); + expect(lines()).toEqual(["value(): 1", "show(): false pending", "hidden"]); + + await vi.advanceTimersByTimeAsync(2000); + flush(); + expect(lines()).toEqual(["value(): 1", "show(): true", "1"]); + } finally { + localDispose(); + } + }); + test("implicit route transition stays held after lazy component is cached", async () => { let setRoute!: (value: "home" | "profile") => void; const localDiv = document.createElement("div");