From b8b48ce0099ef728f3e3e5369fb905ef8b8542ae Mon Sep 17 00:00:00 2001 From: Ryan Carniato Date: Sun, 30 Aug 2026 21:15:00 -0700 Subject: [PATCH 1/2] fix: dedupe navigation against the pending/committed target, not stale reads Co-authored-by: Cursor --- .changeset/fix-navigate-commit-window-drop.md | 5 + src/routing.ts | 34 ++- test/navigate-commit.spec.tsx | 253 ++++++++++++++++++ 3 files changed, 290 insertions(+), 2 deletions(-) create mode 100644 .changeset/fix-navigate-commit-window-drop.md create mode 100644 test/navigate-commit.spec.tsx diff --git a/.changeset/fix-navigate-commit-window-drop.md b/.changeset/fix-navigate-commit-window-drop.md new file mode 100644 index 00000000..6ca6d32f --- /dev/null +++ b/.changeset/fix-navigate-commit-window-drop.md @@ -0,0 +1,5 @@ +--- +"@solidjs/router": patch +--- + +Fix soft navigations being silently dropped in the commit window. Signal reads only see flushed state, so a `navigate()` racing a pending target's unflushed write — or issued right after the commit microtask, before the source write flushed — deduped against a stale location and was lost, breaking last-call-wins. The dedupe now compares against the pending transition target when one exists, and remembers the just-committed target until the reactive read catches up. diff --git a/src/routing.ts b/src/routing.ts index 077717a5..82e9ffb3 100644 --- a/src/routing.ts +++ b/src/routing.ts @@ -780,6 +780,14 @@ export function createRouterContext( // Keep track of last target, so that last call to navigate wins let lastTransitionTarget: LocationChange | undefined; + // Signal writes are forks that reads only see once flushed, so effective() + // lags the router's own actions by up to a flush. A navigate() issued in + // that window would compare against — and push a referrer for — a location + // the router has already left (#3107). `committed` remembers navigateEnd's + // write until effective() catches up (or moves for any other reason, e.g. + // a native pop), so the dedupe below always sees the real location. + let committed: { from: LocationChange; to: LocationChange } | undefined; + // source() remains canonical for native history changes; navigateTarget() // temporarily overrides it for in-flight programmatic navigation. const effective = createMemo(() => navigateTarget() ?? source()); @@ -959,9 +967,27 @@ export function createRouterContext( throw new Error("Too many redirects"); } - const current = effective(); + let current = effective(); + if (committed) { + if (current.value === committed.from.value && current.state === committed.from.state) { + // navigateEnd's source write hasn't flushed yet; reads still answer + // the pre-commit location. The commit target is where we really are. + current = committed.to; + } else { + // effective() moved (the write landed, or a native pop superseded + // it) — the reactive read is authoritative again. + committed = undefined; + } + } + + // Dedupe against where the router is headed, not just where it sits: a + // pending target is a plain variable and never lags, while a navigate() + // racing the pending target's unflushed navigateTarget write would + // otherwise compare against the old location and be silently dropped — + // breaking last-call-wins (#3107). + const headed = lastTransitionTarget ?? current; - if (resolvedTo !== current.value || nextState !== current.state) { + if (resolvedTo !== headed.value || nextState !== headed.state) { if (isServer) { const e = getRequestEvent(); e && (e.response = { status: 302, headers: new Headers({ Location: resolvedTo }) }); @@ -1012,6 +1038,10 @@ export function createRouterContext( function navigateEnd(next: LocationChange) { const first = referrers[0]; if (first) { + // Captured before the write: effective() still reads the pre-commit + // location, which is exactly the stale answer navigateFromRoute's + // dedupe needs to recognize (and see past) until the flush. + committed = { from: untrack(effective), to: next }; setSource({ ...next, replace: first.replace, diff --git a/test/navigate-commit.spec.tsx b/test/navigate-commit.spec.tsx new file mode 100644 index 00000000..7d91df92 --- /dev/null +++ b/test/navigate-commit.spec.tsx @@ -0,0 +1,253 @@ +/** + * solidjs/solid#3107: navigateFromRoute commits the URL from a cancelable + * queueMicrotask guarded by `lastTransitionTarget` identity. The report + * claims a soft navigation can be *silently dropped* — no URL change, no + * error — under timing-sensitive conditions. These tests hammer every + * supersede window the scheduling exposes and assert the last navigation + * always commits: same-tick doubles, reentrant navigation from the + * isRouting flush, microtask interleavings around the commit task, and + * supersede across a parked (async-held) navigation transition. + */ +import { render } from "@solidjs/web"; +import { createEffect, createMemo, Loading, untrack } from "solid-js"; +import { + createRouter, + memoryHistory, + useIsRouting, + useNavigate, + type Navigator +} from "../src/index.js"; + +const settle = async (ms = 5) => { + await new Promise(resolve => queueMicrotask(resolve)); + await new Promise(resolve => setTimeout(resolve, ms)); +}; + +function mount(routes: any, history: ReturnType) { + const div = document.createElement("div"); + document.body.appendChild(div); + const Router = createRouter({ routes, history }); + const dispose = render(() => , div); + return { + div, + text: () => div.textContent, + cleanup: () => { + dispose(); + div.remove(); + } + }; +} + +describe("navigateFromRoute never drops the last navigation", () => { + const originalScrollTo = window.scrollTo; + beforeEach(() => { + window.scrollTo = vi.fn(); + }); + afterAll(() => { + window.scrollTo = originalScrollTo; + }); + + const page = (name: string) => () =>
{name}
; + + function threePages(capture: (nav: Navigator) => void) { + return [ + { + path: "/", + component: () => { + capture(useNavigate()); + return
home
; + } + }, + { path: "/a", component: page("a") }, + { path: "/b", component: page("b") } + ] as const; + } + + test("same-tick double navigation commits the second target", async () => { + const history = memoryHistory(); + let navigate!: Navigator; + const app = mount( + threePages(n => (navigate = n)), + history + ); + try { + navigate("/a"); + navigate("/b"); + await settle(); + expect(history.get()).toBe("/b"); + expect(app.text()).toBe("b"); + } finally { + app.cleanup(); + } + }); + + test("same-tick same-value double navigation still commits", async () => { + const history = memoryHistory(); + let navigate!: Navigator; + const app = mount( + threePages(n => (navigate = n)), + history + ); + try { + navigate("/a"); + navigate("/a"); + await settle(); + expect(history.get()).toBe("/a"); + expect(app.text()).toBe("a"); + } finally { + app.cleanup(); + } + }); + + test("a navigation issued from a microtask racing the commit task wins", async () => { + const history = memoryHistory(); + let navigate!: Navigator; + const app = mount( + threePages(n => (navigate = n)), + history + ); + try { + // registered BEFORE the first navigate, so it runs between the + // first call's setNavigateTarget and its commit microtask + queueMicrotask(() => navigate("/b")); + navigate("/a"); + await settle(); + expect(history.get()).toBe("/b"); + expect(app.text()).toBe("b"); + } finally { + app.cleanup(); + } + }); + + test("a navigation issued after the commit task re-navigates cleanly", async () => { + const history = memoryHistory(); + let navigate!: Navigator; + const app = mount( + threePages(n => (navigate = n)), + history + ); + try { + navigate("/a"); + queueMicrotask(() => navigate("/b")); + await settle(); + expect(history.get()).toBe("/b"); + expect(app.text()).toBe("b"); + } finally { + app.cleanup(); + } + }); + + test("reentrant navigation from the isRouting flush supersedes and commits", async () => { + // navigateFromRoute's firstNavigation branch flushes synchronously so + // pending-state effects can run; an effect that navigates from that + // flush overwrites lastTransitionTarget while the outer call is still + // on the stack — the drop-shaped window from the report's question 1. + const history = memoryHistory(); + let navigate!: Navigator; + let redirected = false; + const routes = [ + { + path: "/", + component: () => { + navigate = useNavigate(); + const isRouting = useIsRouting(); + createEffect( + () => isRouting(), + routing => { + if (routing && !redirected) { + redirected = true; + untrack(() => navigate("/b", { replace: true })); + } + } + ); + return
home
; + } + }, + { path: "/a", component: page("a") }, + { path: "/b", component: page("b") } + ] as const; + const app = mount(routes, history); + try { + await settle(); + navigate("/a"); + await settle(); + expect(redirected).toBe(true); + expect(history.get()).toBe("/b"); + expect(app.text()).toBe("b"); + } finally { + app.cleanup(); + } + }); + + test("superseding a navigation parked on async data still commits the second target", async () => { + // The flight-consumer shape: a navigation transition held open by + // unresolved async while another navigation lands. The held fork must + // not swallow the later commit. + const history = memoryHistory(); + let navigate!: Navigator; + let releaseSlow!: () => void; + const gate = new Promise(resolve => (releaseSlow = resolve)); + const routes = [ + { + path: "/", + component: () => { + navigate = useNavigate(); + return
home
; + } + }, + { + path: "/slow", + component: () => { + const value = createMemo(async () => { + await gate; + return "slow"; + }); + return ( + loading}> +
{value()}
+ + ); + } + }, + { path: "/b", component: page("b") } + ] as const; + const app = mount(routes, history); + try { + navigate("/slow"); + await settle(); + navigate("/b"); + await settle(); + expect(history.get()).toBe("/b"); + expect(app.text()).toBe("b"); + releaseSlow(); + await settle(); + // the late-resolving fork must not resurrect /slow + expect(history.get()).toBe("/b"); + expect(app.text()).toBe("b"); + } finally { + app.cleanup(); + } + }); + + test("rapid-fire navigation bursts always land on the last target", async () => { + const history = memoryHistory(); + let navigate!: Navigator; + const app = mount( + threePages(n => (navigate = n)), + history + ); + try { + for (let round = 0; round < 20; round++) { + const target = round % 2 ? "/a" : "/b"; + navigate("/a"); + queueMicrotask(() => navigate("/b")); + navigate(target); + queueMicrotask(() => navigate(target)); + await settle(1); + expect(history.get()).toBe(target); + } + } finally { + app.cleanup(); + } + }); +}); From 8b3a8c2db562e98985def06ab7fe87e010fc12ed Mon Sep 17 00:00:00 2001 From: Ryan Carniato Date: Mon, 31 Aug 2026 00:35:04 -0700 Subject: [PATCH 2/2] fix: rebuild navigation commitment on transition engine Use Solid's canonical transition state so winning navigations settle before history commits without router-owned shadow lifecycle state. Co-authored-by: Cursor --- .changeset/fix-navigate-commit-window-drop.md | 2 +- src/routers/factory.tsx | 36 ++- src/routing.ts | 251 ++++++++---------- src/types.ts | 10 +- test/lazy-routes.spec.tsx | 4 +- test/navigate-commit.spec.tsx | 229 ++++++++++++++-- test/router.spec.ts | 8 +- 7 files changed, 370 insertions(+), 170 deletions(-) diff --git a/.changeset/fix-navigate-commit-window-drop.md b/.changeset/fix-navigate-commit-window-drop.md index 6ca6d32f..1733628c 100644 --- a/.changeset/fix-navigate-commit-window-drop.md +++ b/.changeset/fix-navigate-commit-window-drop.md @@ -2,4 +2,4 @@ "@solidjs/router": patch --- -Fix soft navigations being silently dropped in the commit window. Signal reads only see flushed state, so a `navigate()` racing a pending target's unflushed write — or issued right after the commit microtask, before the source write flushed — deduped against a stale location and was lost, breaking last-call-wins. The dedupe now compares against the pending transition target when one exists, and remembers the just-committed target until the reactive read catches up. +Rebuild navigation commitment on Solid's transition engine. Programmatic and native navigation now write one canonical location source, repeated writes use engine last-write-wins, and history updates only after the winning transition settles. This fixes dropped soft navigations and native/programmatic races while preserving redirect options, pending link/search state, and lazy-route error delivery. diff --git a/src/routers/factory.tsx b/src/routers/factory.tsx index 4db3b0a4..c341f1ca 100644 --- a/src/routers/factory.tsx +++ b/src/routers/factory.tsx @@ -1,7 +1,15 @@ /*@refresh skip*/ import type { Owner } from "solid-js"; -import { createSignal, getOwner, onCleanup, sharedConfig, untrack } from "solid-js"; +import { + createSignal, + getOwner, + onCleanup, + onSettled, + runWithOwner, + sharedConfig, + untrack +} from "solid-js"; // standalone import: `DEV` is undefined in solid's production build, so app // bundlers fold `DEV &&` diagnostics out of shipped bundles import { DEV } from "solid-js"; @@ -199,27 +207,41 @@ export interface RouterInstance (typeof value === "string" ? { value } : value); const [read, write] = createSignal(wrap(history.get()), { - equals: (a, b) => a.value === b.value && a.state === b.state, + equals: (a, b) => + a.value === b.value && a.state === b.state && a._navigation === b._navigation, ownedWrite: true }); const signal: RouterIntegration["signal"] = [ read, (next: LocationChange) => { - !ignore && history.set(next); if (sharedConfig.registry && !sharedConfig.done) sharedConfig.done = true; write(next); + if (next._navigation && next._navigation > 0) { + // Register out of band so a destination error boundary replacing the + // Router subtree cannot suppress the winning history commit. + runWithOwner(null, () => + onSettled(() => { + if (read() !== next) return; + committing = true; + try { + history.set(next); + } finally { + committing = false; + } + }) + ); + } } ]; history.init && onCleanup( history.init((value = history.get()) => { - ignore = true; - signal[1](wrap(value)); - ignore = false; + if (committing) return; + signal[1]({ ...wrap(value), _navigation: -1 }); }) ); diff --git a/src/routing.ts b/src/routing.ts index 82e9ffb3..1353ead9 100644 --- a/src/routing.ts +++ b/src/routing.ts @@ -1,4 +1,4 @@ -import { Accessor, flush, runWithOwner, type Signal } from "solid-js"; +import { Accessor, runWithOwner, type Signal } from "solid-js"; // standalone import: `DEV` is undefined in solid's production build, so app // bundlers fold `DEV &&` diagnostics out of shipped bundles import { DEV } from "solid-js"; @@ -8,7 +8,9 @@ import { createContext, createMemo, createSignal, + getOwner, isPending, + latest, NotReadyError, onCleanup, untrack, @@ -440,18 +442,15 @@ function getLazyBoundary(thunk: LazyRouteChildren): LazyBoundary { * synchronously once available, the in-flight promise otherwise. * * Failure contract (same as solid's lazy(), 2.0.0-rc.1: the platform - * re-fetches a failed dynamic import): a client rejection is held through its - * settlement flush. The recomputes the settled promise triggers — the parked - * transition's and the mainline commit's — both consume it here as a - * synchronous throw, becoming cached error status that reaches the nearest - * error boundary like a failed lazy() component. The hold clears a microtask - * after the first delivery, so any later recompute — boundary reset(), a new - * navigation — finds a clean record and retries the import. Holding through - * the flush is what keeps the erroring computation from refiring the import - * in a tight loop while it fails. Server records are shared across requests - * and hold nothing — each request retries. Commit is always async — even for - * thunks returning arrays — so the version bump never writes a signal from - * inside a render computation. + * re-fetches a failed dynamic import): each client router's async boundary + * reader retains the rejection as reactive error status, so the destination + * error boundary receives it without refiring the import. The shared record + * holds that rejection through the current turn for concurrent readers, then + * clears so a later tracked retry — boundary reset() or a new navigation — + * starts a fresh import. Server records are shared across requests and hold + * nothing, so each request retries. Commit is always async — even for thunks + * returning arrays — so the version bump never writes a signal from inside a + * render computation. */ export function resolveLazySubtree( record: LazyBoundary @@ -479,7 +478,15 @@ export function resolveLazySubtree( }, e => { // ?? Error(): a held undefined would read as "no failure" and refire - if (!isServer) record.error = e ?? new Error(); + if (!isServer) { + record.error = e ?? new Error(); + record.sweep = true; + // Existing per-router async readers retain the rejection in their + // reactive error status. Hold the shared record through this turn so + // concurrent readers receive the same failure, then permit remounts + // and boundary resets to create a fresh import attempt. + queueMicrotask(() => (record.error = record.sweep = undefined)); + } record.promise = undefined; throw e; } @@ -734,9 +741,9 @@ export function provideFlashDecoder( flashDecoder || (flashDecoder = decoder); } -let intent: Intent | undefined; +let preloadIntent: Intent | undefined; export function getIntent() { - return intent; + return preloadIntent || useOptionalContext(RouterContextObj)?.intent?.(); } let inPreloadFn = false; export function getInPreloadFn() { @@ -767,32 +774,15 @@ export function createRouterContext( if (basePath === undefined) { throw new Error(`${basePath} is not a valid base path`); } else if (basePath && !initialSource.value) { - setSource({ value: basePath, replace: true, scroll: false }); + setSource({ + value: basePath, + replace: true, + scroll: false, + _navigation: isServer ? undefined : 1 + }); } - const [isNavigating, setIsRouting] = createSignal(false, { ownedWrite: true }); - - // Navigate override written from event handlers. - const [navigateTarget, setNavigateTarget] = createSignal(undefined, { - ownedWrite: true - }); - - // Keep track of last target, so that last call to navigate wins - let lastTransitionTarget: LocationChange | undefined; - - // Signal writes are forks that reads only see once flushed, so effective() - // lags the router's own actions by up to a flush. A navigate() issued in - // that window would compare against — and push a referrer for — a location - // the router has already left (#3107). `committed` remembers navigateEnd's - // write until effective() catches up (or moves for any other reason, e.g. - // a native pop), so the dedupe below always sees the real location. - let committed: { from: LocationChange; to: LocationChange } | undefined; - - // source() remains canonical for native history changes; navigateTarget() - // temporarily overrides it for in-flight programmatic navigation. - const effective = createMemo(() => navigateTarget() ?? source()); - const location = createLocation(() => effective().value, () => effective().state, utils.queryWrapper); - const referrers: LocationChange[] = []; + const location = createLocation(() => source().value, () => source().state, utils.queryWrapper); // The flash cookie is consumed eagerly: its one-shot clear (Set-Cookie) // must be appended before streaming flushes the response headers, and an // unread outcome must not haunt a later request's render. Only detection @@ -814,6 +804,26 @@ export function createRouterContext( } let submissions: Signal[]> | undefined; + // NotReadyError's source must be a reactive async node, not the raw + // Promise. Keep one reader per boundary for this router owner so rejection + // is delivered through the graph's error channel and resolution wakes the + // parked matches computation. + const routerOwner = getOwner(); + const lazyReaders = new WeakMap>(); + const readLazySubtree = (record: LazyBoundary) => { + let read = lazyReaders.get(record); + if (!read) { + read = runWithOwner(routerOwner, () => + createMemo(() => { + const result = resolveLazySubtree(record); + return result instanceof Promise ? result.then(() => undefined) : undefined; + }) + ); + lazyReaders.set(record, read); + } + return read(); + }; + const matches = createMemo(() => { const pathname = typeof options.transformUrl === "function" @@ -829,38 +839,46 @@ export function createRouterContext( // inside a boundary just parks the recomputed chain again. const pending = unresolvedLazyMatches(m); if (pending.length) { - const all = Promise.all(pending.map(resolveLazySubtree)); - // pre-handle rejections: the failure reaches the app through this - // computation's error status, so the raw chain must not also surface - // as an unhandled rejection - all.catch(() => {}); - throw new NotReadyError(all); + if (isServer) { + // SSR carries the Promise through NotReadyError so the streaming + // renderer can resume without attempting to serialize route + // definitions (which contain component functions). + const all = Promise.all(pending.map(resolveLazySubtree)); + all.catch(() => {}); + throw new NotReadyError(all); + } else { + // On the client the source must be a reactive async node so transition + // settlement and rejection delivery remain inside the signals graph. + for (const boundary of pending) readLazySubtree(boundary); + } } return m; }); - // Every write is a transition in Solid 2, so a native history pop forks the - // source signal exactly like programmatic navigation does. isRouting is - // therefore derived: the manual flag covers navigateFromRoute's explicit - // window, and isPending over the location/matches read reports any - // in-flight fork — including popstate traversals and the lazy-subtree - // resolution matches() parks on. - const isRouting = createMemo( - () => - isNavigating() || - isPending(() => { - // A real error means the navigation settled (failed) — it surfaces - // to the app through render reads, not through this derivation. - // Not-ready must keep propagating so isPending sees the parking. - try { - matches(); - } catch (e) { - if (e instanceof NotReadyError) throw e; - } - location.search; - location.hash; - }) + const routingPending = createMemo(() => + isPending(() => { + try { + matches(); + } catch (e) { + if (e instanceof NotReadyError) throw e; + } + location.search; + location.hash; + }) ); + const isRouting = () => routingPending() || isPending(source); + + const transitionIntent = (): Intent | undefined => { + if (!isPending(source)) return; + const navigation = latest(source)._navigation; + return navigation === -1 ? "native" : navigation && navigation > 0 ? "navigate" : undefined; + }; + + const pendingNavigation = () => { + if (!isRouting()) return; + const target = latest(source); + return target._navigation && target._navigation > 0 ? target : undefined; + }; const buildParams = () => mergeParams(matches()); @@ -886,8 +904,9 @@ export function createRouterContext( params, wrapParams, isRouting, + intent: transitionIntent, get pendingTarget() { - return lastTransitionTarget; + return pendingNavigation(); }, renderPath, parsePath, @@ -963,29 +982,20 @@ export function createRouterContext( if (resolvedTo === undefined) { throw new Error(`Path '${to}' is not a routable path`); - } else if (referrers.length >= MAX_REDIRECTS) { - throw new Error("Too many redirects"); } - let current = effective(); - if (committed) { - if (current.value === committed.from.value && current.state === committed.from.state) { - // navigateEnd's source write hasn't flushed yet; reads still answer - // the pre-commit location. The commit target is where we really are. - current = committed.to; - } else { - // effective() moved (the write landed, or a native pop superseded - // it) — the reactive read is authoritative again. - committed = undefined; - } - } + const headed = latest(source); + const navigationDepth = + !isServer && + isPending(source) && + headed._navigation !== undefined && + headed._navigation > 0 + ? headed._navigation + : 0; - // Dedupe against where the router is headed, not just where it sits: a - // pending target is a plain variable and never lags, while a navigate() - // racing the pending target's unflushed navigateTarget write would - // otherwise compare against the old location and be silently dropped — - // breaking last-call-wins (#3107). - const headed = lastTransitionTarget ?? current; + if (navigationDepth >= MAX_REDIRECTS) { + throw new Error("Too many redirects"); + } if (resolvedTo !== headed.value || nextState !== headed.state) { if (isServer) { @@ -993,36 +1003,15 @@ export function createRouterContext( e && (e.response = { status: 302, headers: new Headers({ Location: resolvedTo }) }); setSource({ value: resolvedTo, replace, scroll, state: nextState }); } else if (!beforeLeave.current || beforeLeave.current.confirm(resolvedTo, options)) { - referrers.push({ value: current.value, replace, scroll, state: current.state }); - const newTarget: LocationChange = { - value: resolvedTo, - state: nextState - }; - - const firstNavigation = lastTransitionTarget === undefined; - intent = "navigate"; - // assign the target before flushing so effects that run for the - // isRouting flip (e.g. pending link state) can read it - lastTransitionTarget = newTarget; - - if (firstNavigation) { - setIsRouting(true); - flush(); - } - - if (lastTransitionTarget === newTarget) { - setNavigateTarget({ ...lastTransitionTarget }); - - queueMicrotask(() => { - if (lastTransitionTarget !== newTarget) return; - - intent = undefined; - navigateEnd(lastTransitionTarget); - setNavigateTarget(undefined); - setIsRouting(false); - lastTransitionTarget = undefined; - }); - } + runWithOwner(null, () => + setSource({ + value: resolvedTo, + state: nextState, + replace: navigationDepth ? headed.replace : replace, + scroll: navigationDepth ? headed.scroll : scroll, + _navigation: navigationDepth + 1 + }) + ); } } }); @@ -1035,22 +1024,6 @@ export function createRouterContext( navigateFromRoute(route!, to, options); } - function navigateEnd(next: LocationChange) { - const first = referrers[0]; - if (first) { - // Captured before the write: effective() still reads the pre-commit - // location, which is exactly the stale answer navigateFromRoute's - // dedupe needs to recognize (and see past) until the flush. - committed = { from: untrack(effective), to: next }; - setSource({ - ...next, - replace: first.replace, - scroll: first.scroll - }); - referrers.length = 0; - } - } - function preloadRoute(url: URL, preloadData?: boolean) { const matches = getRouteMatches(branches(), url.pathname); // An unresolved lazy subtree in the chain: the placeholder's @@ -1067,8 +1040,8 @@ export function createRouterContext( ); } catch {} } - const prevIntent = intent; - intent = "preload"; + const prevIntent = preloadIntent; + preloadIntent = "preload"; for (let match in matches) { const { route, params } = matches[match]; route.component && @@ -1094,7 +1067,7 @@ export function createRouterContext( ); inPreloadFn = false; } - intent = prevIntent; + preloadIntent = prevIntent; } // Seeds the initial submission from a no-JS form post: the server @@ -1140,7 +1113,9 @@ export function createRouteContext( (component as MaybePreloadableComponent).preload && (component as MaybePreloadableComponent).preload!(); inPreloadFn = true; - const data = preload ? preload({ params, location, intent: intent || "initial" }) : undefined; + const data = preload + ? preload({ params, location, intent: router.intent?.() || "initial" }) + : undefined; inPreloadFn = false; const route: RouteContext = { diff --git a/src/types.ts b/src/types.ts index 70b870bb..79ac3b55 100644 --- a/src/types.ts +++ b/src/types.ts @@ -117,6 +117,8 @@ export interface LocationChange { scroll?: boolean; state?: S; rawPath?: string; + /** @internal Positive for programmatic navigation depth; `-1` for native history. */ + _navigation?: number; } export interface RouterIntegration { // Structural getter/setter pair rather than solid's `Signal` — the server's @@ -342,9 +344,9 @@ export interface LazyBoundary { thunk: LazyRouteChildren; promise?: Promise; resolved?: readonly RouteDefinition[]; - /** A client load failure, held through its settlement flush so every - * recompute it triggers delivers the same error instead of refiring the - * import; cleared a microtask after first delivery so retries can run. */ + /** A client load failure, retained by each router's async reader and held + * here through the rejection turn for concurrent readers; then cleared so + * a later boundary reset or navigation can retry the import. */ error?: unknown; /** The pending clear of `error` has been scheduled. */ sweep?: boolean; @@ -382,6 +384,8 @@ export interface RouterContext { wrapParams: (getParams: () => Params) => Params; navigatorFactory: NavigatorFactory; isRouting: () => boolean; + /** @internal Intent of the in-flight canonical location transition. */ + intent?: () => Intent | undefined; /** The target of the in-flight navigation transition, if any. Not reactive. */ readonly pendingTarget?: LocationChange; matches: () => RouteMatch[]; diff --git a/test/lazy-routes.spec.tsx b/test/lazy-routes.spec.tsx index 10130781..aad12bbe 100644 --- a/test/lazy-routes.spec.tsx +++ b/test/lazy-routes.spec.tsx @@ -109,6 +109,7 @@ describe("lazy route subtrees", () => { return
Home
; }; + const history = memoryHistory(); const Router = createRouter({ routes: [ { path: "/", component: Home }, @@ -123,7 +124,7 @@ describe("lazy route subtrees", () => { } } ] as const, - history: memoryHistory() + history }); let resetBoundary!: () => void; @@ -149,6 +150,7 @@ describe("lazy route subtrees", () => { expect(div.querySelector('[data-route="plugin-home"]')).toBeFalsy(); expect(div.querySelector('[data-route="error"]')).toBeTruthy(); expect((caught[0] as Error).message).toBe("chunk load failed"); + expect(history.get()).toBe("/plugins"); // the failure is not cached across attempts: resetting the boundary // remounts the router at /plugins — a fresh attempt that re-fetches diff --git a/test/navigate-commit.spec.tsx b/test/navigate-commit.spec.tsx index 7d91df92..746aa7c6 100644 --- a/test/navigate-commit.spec.tsx +++ b/test/navigate-commit.spec.tsx @@ -1,22 +1,21 @@ /** - * solidjs/solid#3107: navigateFromRoute commits the URL from a cancelable - * queueMicrotask guarded by `lastTransitionTarget` identity. The report - * claims a soft navigation can be *silently dropped* — no URL change, no - * error — under timing-sensitive conditions. These tests hammer every - * supersede window the scheduling exposes and assert the last navigation - * always commits: same-tick doubles, reentrant navigation from the - * isRouting flush, microtask interleavings around the commit task, and - * supersede across a parked (async-held) navigation transition. + * solidjs/solid#3107: navigation commitment must remain last-write-wins + * across same-tick calls, microtask interleavings, reentrant pending-state + * navigation, native traversal, and async-held transitions. History follows + * only the canonical source write that actually lands. */ import { render } from "@solidjs/web"; import { createEffect, createMemo, Loading, untrack } from "solid-js"; +import { vi } from "vitest"; import { createRouter, memoryHistory, useIsRouting, useNavigate, - type Navigator + type Navigator, + type RouteDefinition } from "../src/index.js"; +import { useRouter } from "../src/routing.js"; const settle = async (ms = 5) => { await new Promise(resolve => queueMicrotask(resolve)); @@ -138,13 +137,13 @@ describe("navigateFromRoute never drops the last navigation", () => { }); test("reentrant navigation from the isRouting flush supersedes and commits", async () => { - // navigateFromRoute's firstNavigation branch flushes synchronously so - // pending-state effects can run; an effect that navigates from that - // flush overwrites lastTransitionTarget while the outer call is still - // on the stack — the drop-shaped window from the report's question 1. + // A pending-state effect can navigate while the first target is held. + // The second source write must supersede the first transition. const history = memoryHistory(); let navigate!: Navigator; let redirected = false; + let resolveA!: (routes: { default: RouteDefinition[] }) => void; + const lazyA = new Promise<{ default: RouteDefinition[] }>(resolve => (resolveA = resolve)); const routes = [ { path: "/", @@ -163,7 +162,11 @@ describe("navigateFromRoute never drops the last navigation", () => { return
home
; } }, - { path: "/a", component: page("a") }, + { + path: "/a", + component: (props: any) => <>{props.children}, + children: () => lazyA + }, { path: "/b", component: page("b") } ] as const; const app = mount(routes, history); @@ -174,6 +177,9 @@ describe("navigateFromRoute never drops the last navigation", () => { expect(redirected).toBe(true); expect(history.get()).toBe("/b"); expect(app.text()).toBe("b"); + resolveA({ default: [{ path: "/", component: page("a") }] }); + await settle(); + expect(history.get()).toBe("/b"); } finally { app.cleanup(); } @@ -229,6 +235,195 @@ describe("navigateFromRoute never drops the last navigation", () => { } }); + test("superseding redirects commit once with the first navigation's history policy", async () => { + const history = memoryHistory(); + const set = vi.spyOn(history, "set"); + let navigate!: Navigator; + const app = mount( + threePages(n => (navigate = n)), + history + ); + try { + navigate("/a", { replace: true, scroll: false }); + navigate("/b", { replace: false, scroll: true }); + await settle(); + + expect(set).toHaveBeenCalledTimes(1); + expect(set).toHaveBeenCalledWith( + expect.objectContaining({ + value: "/b", + replace: true, + scroll: false + }) + ); + } finally { + app.cleanup(); + } + }); + + test("history waits for a parked lazy route to settle", async () => { + const history = memoryHistory(); + let navigate!: Navigator; + let resolveRoutes!: (routes: { default: RouteDefinition[] }) => void; + const lazyRoutes = new Promise<{ default: RouteDefinition[] }>(resolve => { + resolveRoutes = resolve; + }); + const routes = [ + { + path: "/", + component: () => { + navigate = useNavigate(); + return
home
; + } + }, + { + path: "/lazy", + component: (props: any) => <>{props.children}, + children: () => lazyRoutes + } + ] as const; + const app = mount(routes, history); + try { + navigate("/lazy"); + await settle(); + + expect(history.get()).toBe("/"); + expect(app.text()).toBe("home"); + + resolveRoutes({ + default: [{ path: "/", component: page("lazy") }] + }); + await settle(); + + expect(history.get()).toBe("/lazy"); + expect(app.text()).toBe("lazy"); + } finally { + app.cleanup(); + } + }); + + test("a native traversal supersedes a pending programmatic navigation", async () => { + const history = memoryHistory(); + history.set({ value: "/a" }); + history.set({ value: "/b" }); + history.back(); + + let navigate!: Navigator; + let releaseSlow!: () => void; + const gate = new Promise(resolve => (releaseSlow = resolve)); + const routes = [ + { + path: "/a", + component: () => { + navigate = useNavigate(); + return
a
; + } + }, + { path: "/b", component: page("b") }, + { + path: "/slow", + component: () => { + const value = createMemo(async () => { + await gate; + return "slow"; + }); + return ( + loading}> +
{value()}
+ + ); + } + } + ] as const; + const app = mount(routes, history); + try { + navigate("/slow"); + history.forward(); + await settle(); + + expect(history.get()).toBe("/b"); + expect(app.text()).toBe("b"); + + releaseSlow(); + await settle(); + + expect(history.get()).toBe("/b"); + expect(app.text()).toBe("b"); + } finally { + app.cleanup(); + } + }); + + test("navigation intent is derived from the pending source transition", async () => { + const history = memoryHistory(); + let navigate!: Navigator; + let currentIntent!: () => string | undefined; + const seen: string[] = []; + const routes = [ + { + path: "/", + component: () => { + navigate = useNavigate(); + currentIntent = useRouter().intent!; + return
home
; + } + }, + { + path: "/slow", + preload: ({ intent }: any) => seen.push(intent), + component: () => { + const value = createMemo(async () => { + await new Promise(resolve => setTimeout(resolve, 25)); + return "slow"; + }); + return ( + loading}> +
{value()}
+ + ); + } + } + ] as const; + const app = mount(routes, history); + try { + navigate("/slow"); + expect(currentIntent()).toBe("navigate"); + await settle(); + expect(seen).toEqual(["navigate"]); + + await settle(30); + expect(currentIntent()).toBeUndefined(); + expect(history.get()).toBe("/slow"); + } finally { + app.cleanup(); + } + }); + + test("native traversals expose native intent while matching", async () => { + const history = memoryHistory(); + history.set({ value: "/native" }); + history.back(); + const seen: string[] = []; + const routes = [ + { path: "/", component: page("home") }, + { + path: "/native", + preload: ({ intent }: any) => seen.push(intent), + component: page("native") + } + ] as const; + const app = mount(routes, history); + try { + history.forward(); + await settle(); + expect(seen).toEqual(["native"]); + expect(history.get()).toBe("/native"); + expect(app.text()).toBe("native"); + } finally { + app.cleanup(); + } + }); + test("rapid-fire navigation bursts always land on the last target", async () => { const history = memoryHistory(); let navigate!: Navigator; @@ -237,8 +432,10 @@ describe("navigateFromRoute never drops the last navigation", () => { history ); try { - for (let round = 0; round < 20; round++) { - const target = round % 2 ? "/a" : "/b"; + let seed = 0x3107; + for (let round = 0; round < 200; round++) { + seed = (seed * 1664525 + 1013904223) >>> 0; + const target = seed & 1 ? "/a" : "/b"; navigate("/a"); queueMicrotask(() => navigate("/b")); navigate(target); diff --git a/test/router.spec.ts b/test/router.spec.ts index 2a959ed8..7fba05fa 100644 --- a/test/router.spec.ts +++ b/test/router.spec.ts @@ -267,7 +267,7 @@ describe("Router should", () => { navigate("/foo/bar"); waitFor(() => signal[0]().value === "/foo/bar").then(n => { - expect(n).toBe(1); + expect(n).toBe(0); expect(signal[0]().replace).not.toBe(true); resolve(); }); @@ -285,7 +285,7 @@ describe("Router should", () => { navigate("/foo", { state }); waitFor(() => signal[0]().value === "/foo").then(n => { - expect(n).toBe(1); + expect(n).toBe(0); expect(location.state).toEqual(state); resolve(); }); @@ -303,7 +303,7 @@ describe("Router should", () => { navigate("/", { state }); waitFor(() => signal[0]().state === state).then(n => { - expect(n).toBe(1); + expect(n).toBe(0); expect(location.state).toEqual(state); resolve(); }); @@ -325,7 +325,7 @@ describe("Router should", () => { navigate("/foo/5"); waitFor(() => signal[0]().value === "/foo/5").then(n => { - expect(n).toBe(1); + expect(n).toBe(0); expect(signal[0]().replace).not.toBe(true); resolve(); });