diff --git a/.changeset/fix-stale-params-on-navigation.md b/.changeset/fix-stale-params-on-navigation.md new file mode 100644 index 00000000..a1198e34 --- /dev/null +++ b/.changeset/fix-stale-params-on-navigation.md @@ -0,0 +1,7 @@ +--- +"@solidjs/router": patch +--- + +Fix params becoming undefined in outgoing components during navigation. + +Route roots are now created under the `Routes` component owner so they survive re-runs of the route state memo (Solid 2 disposes roots created inside a computation when it re-runs). Each route context also gets params scoped to its own lifetime: they retain their last values while the route is being torn down, so effects and async fetches in outgoing components (e.g. kept alive by a `` boundary) never observe another route's params. This also removes the incorrect fallback that could hand a route context another route's match during teardown. diff --git a/src/routers/components.tsx b/src/routers/components.tsx index 40e82e7d..af559498 100644 --- a/src/routers/components.tsx +++ b/src/routers/components.tsx @@ -1,7 +1,7 @@ /*@refresh skip*/ import type {Component, JSX, Owner} from "solid-js"; -import {children, createMemo, createRoot, getOwner, merge, untrack} from "solid-js"; +import {children, createMemo, createRoot, getOwner, merge, runWithOwner, untrack} from "solid-js"; import {getRequestEvent, isServer, type RequestEvent} from "@solidjs/web"; import { createBranches, @@ -117,6 +117,10 @@ function Routes(props: { routerState: RouterContext; branches: Branch[] }) { const disposers: (() => void)[] = []; let root: RouteContext | undefined; let prevMatches: RouteMatch[] | undefined; + // Route roots must outlive re-runs of the `routeStates` memo below, so they + // are created under the owner of this component rather than the memo's + // computation (which disposes its children every time it re-runs). + const owner = getOwner()!; const routeStates = createMemo((prev: RouteContext[] | undefined) => { const nextMatches = props.routerState.matches(); @@ -135,18 +139,27 @@ function Routes(props: { routerState: RouterContext; branches: Branch[] }) { disposers[i](); } - createRoot(dispose => { - disposers[i] = dispose; - next[i] = createRouteContext( - props.routerState, - next[i - 1] || props.routerState.base, - createOutlet(() => routeStates()?.[i + 1]), - () => { + runWithOwner(owner, () => + createRoot(dispose => { + disposers[i] = dispose; + const routeKey = nextMatch.route.key; + // Retain the last matches in which this route participated so + // that its components and preloads never observe another + // route's params/path while this route is being torn down. + const matchesAtLevel = createMemo((prev?: RouteMatch[]) => { const routeMatches = props.routerState.matches(); - return routeMatches[i] ?? routeMatches[0]; - } - ); - }); + const m = routeMatches[i]; + return m && m.route.key === routeKey ? routeMatches : prev || nextMatches; + }); + next[i] = createRouteContext( + props.routerState, + next[i - 1] || props.routerState.base, + createOutlet(() => routeStates()?.[i + 1]), + () => matchesAtLevel()[i], + matchesAtLevel + ); + }) + ); } } diff --git a/src/routing.ts b/src/routing.ts index 06e659e6..043e90f4 100644 --- a/src/routing.ts +++ b/src/routing.ts @@ -196,7 +196,7 @@ export const useCurrentMatches = () => useRouter().matches; * const getUser = query(() => fetchUser(params.id), "user"); * ``` */ -export const useParams = () => useRouter().params as T; +export const useParams = () => useRoute().params as T; /** * Retrieves a tuple containing a reactive object to read the current location's query parameters and a method to update them. @@ -378,6 +378,14 @@ export function getRouteMatches(branches: Branch[], location: string): RouteMatc return []; } +function mergeParams(matches: RouteMatch[]): Params { + const params: Params = {}; + for (let i = 0; i < matches.length; i++) { + Object.assign(params, matches[i].params); + } + return params; +} + function createLocation( path: Accessor, state: Accessor, @@ -487,21 +495,17 @@ export function createRouterContext( return getRouteMatches(branches(), location.pathname); }); - const buildParams = () => { - const m = matches(); - const params: Params = {}; - for (let i = 0; i < m.length; i++) { - Object.assign(params, m[i].params); - } - return params; - }; + const buildParams = () => mergeParams(matches()); + + const wrapParams = utils.paramsWrapper + ? (getParams: () => Params) => utils.paramsWrapper!(getParams, branches) + : (getParams: () => Params) => createMemoObject(getParams); - const params = utils.paramsWrapper - ? utils.paramsWrapper(buildParams, branches) - : createMemoObject(buildParams); + const params = wrapParams(buildParams); const baseRoute: RouteContext = { pattern: basePath, + params, path: () => basePath, outlet: () => null, resolvePath(to: string) { @@ -513,6 +517,7 @@ export function createRouterContext( base: baseRoute, location, params, + wrapParams, isRouting, renderPath, parsePath, @@ -673,11 +678,16 @@ export function createRouteContext( router: RouterContext, parent: RouteContext, outlet: () => JSX.Element, - match: () => RouteMatch + match: () => RouteMatch, + matches: () => RouteMatch[] = () => [match()] ): RouteContext { - const { base, location, params } = router; + const { base, location, wrapParams } = router; const { pattern, component, preload } = match().route; const path = createMemo(() => match().path); + // Params scoped to this route's lifetime. `matches` is expected to retain + // its last valid value while this route is being torn down, so outgoing + // components and preloads never observe another route's params. + const params = wrapParams(() => mergeParams(matches())); component && (component as MaybePreloadableComponent).preload && @@ -689,6 +699,7 @@ export function createRouteContext( const route: RouteContext = { parent, pattern, + params, path, outlet: () => component diff --git a/src/types.ts b/src/types.ts index 9b8d1ab2..bb30eece 100644 --- a/src/types.ts +++ b/src/types.ts @@ -155,6 +155,7 @@ export interface RouteContext { parent?: RouteContext; child?: RouteContext; pattern: string; + params: Params; path: () => string; outlet: () => JSX.Element; resolvePath(to: string): string | undefined; @@ -173,6 +174,7 @@ export interface RouterContext { base: RouteContext; location: Location; params: Params; + wrapParams: (getParams: () => Params) => Params; navigatorFactory: NavigatorFactory; isRouting: () => boolean; matches: () => RouteMatch[]; diff --git a/test/client-navigation.spec.tsx b/test/client-navigation.spec.tsx index 807e9f1a..cb4c39ea 100644 --- a/test/client-navigation.spec.tsx +++ b/test/client-navigation.spec.tsx @@ -1,11 +1,31 @@ import { render } from "@solidjs/web"; +import { createEffect, createMemo, Loading } from "solid-js"; import { vi } from "vitest"; -import { A, MemoryRouter, Route } from "../src/index.js"; +import { + A, + MemoryRouter, + Route, + createMemoryHistory, + useNavigate, + useParams, + type Navigator +} from "../src/index.js"; + +const settle = async (ms = 0) => { + await new Promise(resolve => queueMicrotask(() => resolve())); + await new Promise(resolve => setTimeout(resolve, ms)); +}; describe("Client navigation should", () => { - test("update rendered routes after clicking links", async () => { - const originalScrollTo = window.scrollTo; + const originalScrollTo = window.scrollTo; + beforeEach(() => { window.scrollTo = vi.fn(); + }); + afterAll(() => { + window.scrollTo = originalScrollTo; + }); + + test("update rendered routes after clicking links", async () => { const div = document.createElement("div"); document.body.appendChild(div); @@ -52,7 +72,221 @@ describe("Client navigation should", () => { } finally { dispose(); div.remove(); - window.scrollTo = originalScrollTo; + } + }); + + test("keep params stable in outgoing components when navigating away", async () => { + const div = document.createElement("div"); + document.body.appendChild(div); + + const observed: (string | undefined)[][] = []; + let navigate!: Navigator; + + const history = createMemoryHistory(); + history.set({ value: "/users/1/comments" }); + + const Users = () => { + const params = useParams(); + navigate = useNavigate(); + createEffect( + () => [params.id, params.view] as const, + ([id, view]) => { + observed.push([id, view]); + } + ); + return
{params.id}
; + }; + + const dispose = render( + () => ( + + +
About page
} /> +
+ ), + div + ); + + try { + await settle(); + expect(div.querySelector('[data-route="users"]')?.textContent).toBe("1"); + expect(observed).toEqual([["1", "comments"]]); + + // Same-route param navigation should update params. + navigate("/users/2/likes"); + await settle(); + expect(div.querySelector('[data-route="users"]')?.textContent).toBe("2"); + expect(observed).toEqual([ + ["1", "comments"], + ["2", "likes"] + ]); + + // Navigating away must not re-run the outgoing route's effects with + // another route's (empty) params. + navigate("/about"); + await settle(); + expect(div.querySelector('[data-route="about"]')?.textContent).toBe("About page"); + expect(div.querySelector('[data-route="users"]')).toBeNull(); + expect(observed).toEqual([ + ["1", "comments"], + ["2", "likes"] + ]); + } finally { + dispose(); + div.remove(); + } + }); + + test("not refetch with empty params in outgoing components suspended by a Loading boundary", async () => { + const div = document.createElement("div"); + document.body.appendChild(div); + + const fetched: (string | undefined)[][] = []; + let navigate!: Navigator; + + const history = createMemoryHistory(); + history.set({ value: "/users/1/comments" }); + + const Users = () => { + const params = useParams(); + navigate = useNavigate(); + const data = createMemo(async () => { + fetched.push([params.id, params.view]); + return `${params.id}-${params.view}`; + }); + return
{data()}
; + }; + + const About = () => { + const data = createMemo(async () => { + await new Promise(resolve => setTimeout(resolve, 25)); + return "About page"; + }); + return
{data()}
; + }; + + const dispose = render( + () => ( + ( + loading}>{props.children} + )} + > + + + + ), + div + ); + + try { + await settle(10); + expect(div.querySelector('[data-route="users"]')?.textContent).toBe("1-comments"); + expect(fetched).toEqual([["1", "comments"]]); + + // Navigating to a suspending route keeps the outgoing tree alive while + // the new one settles; it must not refetch with the new route's params. + navigate("/about"); + await settle(50); + expect(div.querySelector('[data-route="about"]')?.textContent).toBe("About page"); + expect(fetched).toEqual([["1", "comments"]]); + } finally { + dispose(); + div.remove(); + } + }); + + test("expose child params to parent layouts", async () => { + const div = document.createElement("div"); + document.body.appendChild(div); + + let navigate!: Navigator; + + const history = createMemoryHistory(); + history.set({ value: "/users/1" }); + + const Layout = (props: { children?: any }) => { + const params = useParams(); + navigate = useNavigate(); + return ( + <> +
{params.id ?? "none"}
+ {props.children} + + ); + }; + + const dispose = render( + () => ( + + +
Index
} /> +
User
} /> +
+
+ ), + div + ); + + try { + await settle(); + expect(div.querySelector("[data-layout-id]")?.textContent).toBe("1"); + + navigate("/users/2"); + await settle(); + expect(div.querySelector("[data-layout-id]")?.textContent).toBe("2"); + + navigate("/users"); + await settle(); + expect(div.querySelector('[data-route="index"]')).toBeTruthy(); + expect(div.querySelector("[data-layout-id]")?.textContent).toBe("none"); + } finally { + dispose(); + div.remove(); + } + }); + + test("call preload with the route's own params during navigation", async () => { + const div = document.createElement("div"); + document.body.appendChild(div); + + const preloadedParams: (string | undefined)[] = []; + let navigate!: Navigator; + + const history = createMemoryHistory(); + history.set({ value: "/users/1" }); + + const Home = () => { + navigate = useNavigate(); + return
Users
; + }; + + const dispose = render( + () => ( + + + { + preloadedParams.push(params.postId); + }} + component={() =>
Post
} + /> +
+ ), + div + ); + + try { + await settle(); + navigate("/posts/42"); + await settle(); + expect(div.querySelector('[data-route="post"]')).toBeTruthy(); + expect(preloadedParams).toEqual(["42"]); + } finally { + dispose(); + div.remove(); } }); });