Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-conditional-async-reveal-pending.md
Original file line number Diff line number Diff line change
@@ -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.
39 changes: 25 additions & 14 deletions packages/signals/src/core/scheduler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
139 changes: 139 additions & 0 deletions packages/signals/tests/spec-async-semantics.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
49 changes: 49 additions & 0 deletions packages/web/test/loading.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<div>
<div>
value(): {value()} {isPending(value) ? "pending" : null}
</div>
<div>
show(): {String(show())} {isPending(show) ? "pending" : null}
</div>
<div>{show() ? details() : "hidden"}</div>
</div>
);
}, 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");
Expand Down
Loading