diff --git a/.changeset/unlucky-keys-collect.md b/.changeset/unlucky-keys-collect.md new file mode 100644 index 0000000000..f69f7fb700 --- /dev/null +++ b/.changeset/unlucky-keys-collect.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Remove internal `discoveredRoutes` FIFO queue from `unstable_patchRoutesOnNavigation` diff --git a/docs/routers/create-browser-router.md b/docs/routers/create-browser-router.md index 31700b4bc9..112992b333 100644 --- a/docs/routers/create-browser-router.md +++ b/docs/routers/create-browser-router.md @@ -597,6 +597,88 @@ let router = createBrowserRouter( ); ``` +### A note on routes with parameters + +Because React Router uses ranked routes to find the best match for a given path, there is an interesting ambiguity introduced when only a partial route tree is known at any given point in time. If we match a fully static route such as `path: "/about/contact-us"` then we know we've found the right match since it's composed entirely of static URL segments, and thus we do not need to bother asking for any other potentially higher-scoring routes. + +However, routes with parameters (dynamic or splat) can't make this assumption because there might be a not-yet-discovered route tht scores higher. Consider a full route tree such as: + +```js +// Assume this is the full route tree for your app +const routes = [ + { + path: "/", + Component: Home, + }, + { + id: "blog", + path: "/blog", + Component: BlogLayout, + children: [ + { path: "new", Component: NewPost }, + { path: ":slug", Component: BlogPost }, + ], + }, +]; +``` + +And then assume we want to use `patchRoutesOnNavigation` to fill this in as the user navigates around: + +```js +// Start with only the index route +const router = createBrowserRouter( + [ + { + path: "/", + Component: Home, + }, + ], + { + patchRoutesOnNavigation({ path, patch }) { + if (path === "/blog/new") { + patch("blog", [ + { + path: "new", + Component: NewPost, + }, + ]); + } else if (path.startsWith("/blog")) { + patch("blog", [ + { + path: ":slug", + Component: BlogPost, + }, + ]); + } + }, + } +); +``` + +If the user were to a blog post first (i.e., `/blog/my-post`) we would patch in the `:slug` route. Then if the user navigated to `/blog/new` to write a new post, we'd match `/blog/:slug` but it wouldn't be the _right_ match! We need to call `patchRoutesOnNavigation` just in case there exists a higher-scoring route we've not yet discovered, which in this case there is. + +So, anytime React Router matches a path that contains at least one param, it will call `patchRoutesOnNavigation` and match routes again just to confirm it has found the best match. + +If your `patchRoutesOnNavigation` implementation is expensive or making side-effect `fetch` calls to a backend server, you may want to consider tracking previously seen routes to avoid over-fetching in cases where you know the proper route has already been found. This can usually be as simple as maintaining a small cache of prior `path` values for which you've already patched in the right routes: + +```js +let discoveredRoutes = new Set(); + +const router = createBrowserRouter(routes, { + patchRoutesOnNavigation({ path, patch }) { + if (discoveredRoutes.has(path)) { + // We've seen this before so nothing to patch in and we can let the router + // use the routes it already knows about + return; + } + + discoveredRoutes.add(path); + + // ... patch routes in accordingly + }, +}); +``` + ## `opts.window` Useful for environments like browser devtool plugins or testing to use a different window than the global `window`. diff --git a/packages/router/__tests__/lazy-discovery-test.ts b/packages/router/__tests__/lazy-discovery-test.ts index 0fd61d3ca1..b22b59a2af 100644 --- a/packages/router/__tests__/lazy-discovery-test.ts +++ b/packages/router/__tests__/lazy-discovery-test.ts @@ -1259,7 +1259,7 @@ describe("Lazy Route Discovery (Fog of War)", () => { unsubscribe(); }); - it('does not re-call for previously called "good" paths', async () => { + it("does not re-patch previously patched routes", async () => { let count = 0; router = createRouter({ history: createMemoryHistory(), @@ -1267,126 +1267,67 @@ describe("Lazy Route Discovery (Fog of War)", () => { { path: "/", }, - { - id: "param", - path: ":param", - }, ], - async patchRoutesOnNavigation() { + async patchRoutesOnNavigation({ patch }) { count++; + patch(null, [ + { + id: "param", + path: ":param", + }, + ]); await tick(); - // Nothing to patch - there is no better static route in this case }, }); - await router.navigate("/whatever"); - expect(count).toBe(1); - expect(router.state.location.pathname).toBe("/whatever"); + await router.navigate("/a"); + expect(router.state.location.pathname).toBe("/a"); expect(router.state.matches.map((m) => m.route.id)).toEqual(["param"]); - - await router.navigate("/"); expect(count).toBe(1); - expect(router.state.location.pathname).toBe("/"); - - await router.navigate("/whatever"); - expect(count).toBe(1); // Not called again - expect(router.state.location.pathname).toBe("/whatever"); - expect(router.state.matches.map((m) => m.route.id)).toEqual(["param"]); - }); - - it("does not re-call for previously called 404 paths", async () => { - let count = 0; - router = createRouter({ - history: createMemoryHistory(), - routes: [ + expect(router.routes).toMatchInlineSnapshot(` + [ { - id: "index", - path: "/", + "children": undefined, + "hasErrorBoundary": false, + "id": "0", + "path": "/", }, { - id: "static", - path: "static", + "children": undefined, + "hasErrorBoundary": false, + "id": "param", + "path": ":param", }, - ], - async patchRoutesOnNavigation() { - count++; - }, - }); - - await router.navigate("/junk"); - expect(count).toBe(1); - expect(router.state.location.pathname).toBe("/junk"); - expect(router.state.errors?.index).toEqual( - new ErrorResponseImpl( - 404, - "Not Found", - new Error('No route matches URL "/junk"'), - true - ) - ); + ] + `); await router.navigate("/"); - expect(count).toBe(1); expect(router.state.location.pathname).toBe("/"); - expect(router.state.errors).toBeNull(); - - await router.navigate("/junk"); expect(count).toBe(1); - expect(router.state.location.pathname).toBe("/junk"); - expect(router.state.errors?.index).toEqual( - new ErrorResponseImpl( - 404, - "Not Found", - new Error('No route matches URL "/junk"'), - true - ) - ); - }); - it("caps internal fifo queue at 1000 paths", async () => { - let count = 0; - router = createRouter({ - history: createMemoryHistory(), - routes: [ + await router.navigate("/b"); + expect(router.state.location.pathname).toBe("/b"); + expect(router.state.matches.map((m) => m.route.id)).toEqual(["param"]); + expect(router.state.errors).toBeNull(); + // Called again + expect(count).toBe(2); + // But not patched again + expect(router.routes).toMatchInlineSnapshot(` + [ { - path: "/", + "children": undefined, + "hasErrorBoundary": false, + "id": "0", + "path": "/", }, { - id: "param", - path: ":param", + "children": undefined, + "hasErrorBoundary": false, + "id": "param", + "path": ":param", }, - ], - async patchRoutesOnNavigation() { - count++; - // Nothing to patch - there is no better static route in this case - }, - }); - - // Fill it up with 1000 paths - for (let i = 1; i <= 1000; i++) { - await router.navigate(`/path-${i}`); - expect(count).toBe(i); - expect(router.state.location.pathname).toBe(`/path-${i}`); - - await router.navigate("/"); - expect(count).toBe(i); - expect(router.state.location.pathname).toBe("/"); - } - - // Don't call patchRoutesOnNavigation since this is the first item in the queue - await router.navigate(`/path-1`); - expect(count).toBe(1000); - expect(router.state.location.pathname).toBe(`/path-1`); - - // Call patchRoutesOnNavigation and evict the first item - await router.navigate(`/path-1001`); - expect(count).toBe(1001); - expect(router.state.location.pathname).toBe(`/path-1001`); - - // Call patchRoutesOnNavigation since this item was evicted - await router.navigate(`/path-1`); - expect(count).toBe(1002); - expect(router.state.location.pathname).toBe(`/path-1`); + ] + `); }); describe("errors", () => { diff --git a/packages/router/router.ts b/packages/router/router.ts index 8502da54eb..aacc55c1f7 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -814,10 +814,6 @@ export function createRouter(init: RouterInit): Router { let unlistenHistory: (() => void) | null = null; // Externally-provided functions to call on all state changes let subscribers = new Set(); - // FIFO queue of previously discovered routes to prevent re-calling on - // subsequent navigations to the same path - let discoveredRoutesMaxSize = 1000; - let discoveredRoutes = new Set(); // Externally-provided object to hold scroll restoration locations during routing let savedScrollPositions: Record | null = null; // Externally-provided function to get scroll restoration keys @@ -3243,13 +3239,6 @@ export function createRouter(init: RouterInit): Router { pathname: string ): { active: boolean; matches: AgnosticDataRouteMatch[] | null } { if (patchRoutesOnNavigationImpl) { - // Don't bother re-calling patchRouteOnMiss for a path we've already - // processed. the last execution would have patched the route tree - // accordingly so `matches` here are already accurate. - if (discoveredRoutes.has(pathname)) { - return { active: false, matches }; - } - if (!matches) { let fogMatches = matchRoutesImpl( routesToUse, @@ -3333,7 +3322,6 @@ export function createRouter(init: RouterInit): Router { let newMatches = matchRoutes(routesToUse, pathname, basename); if (newMatches) { - addToFifoQueue(pathname, discoveredRoutes); return { type: "success", matches: newMatches }; } @@ -3352,7 +3340,6 @@ export function createRouter(init: RouterInit): Router { (m, i) => m.route.id === newPartialMatches![i].route.id )) ) { - addToFifoQueue(pathname, discoveredRoutes); return { type: "success", matches: null }; } @@ -3360,14 +3347,6 @@ export function createRouter(init: RouterInit): Router { } } - function addToFifoQueue(path: string, queue: Set) { - if (queue.size >= discoveredRoutesMaxSize) { - let first = queue.values().next().value; - queue.delete(first); - } - queue.add(path); - } - function _internalSetRoutes(newRoutes: AgnosticDataRouteObject[]) { manifest = {}; inFlightDataRoutes = convertRoutesToDataRoutes( @@ -4661,32 +4640,42 @@ function patchRoutesImpl( manifest: RouteManifest, mapRouteProperties: MapRoutePropertiesFunction ) { + let childrenToPatch: AgnosticDataRouteObject[]; if (routeId) { let route = manifest[routeId]; invariant( route, `No route found to patch children into: routeId = ${routeId}` ); - let dataChildren = convertRoutesToDataRoutes( - children, - mapRouteProperties, - [routeId, "patch", String(route.children?.length || "0")], - manifest - ); - if (route.children) { - route.children.push(...dataChildren); - } else { - route.children = dataChildren; + if (!route.children) { + route.children = []; } + childrenToPatch = route.children; } else { - let dataChildren = convertRoutesToDataRoutes( - children, - mapRouteProperties, - ["patch", String(routesToUse.length || "0")], - manifest - ); - routesToUse.push(...dataChildren); + childrenToPatch = routesToUse; } + + // Don't patch in routes we already know about so that `patch` is idempotent + // to simplify user-land code. This is useful because we re-call the + // `patchRoutesOnNavigation` function for matched routes with params. + let uniqueChildren = children.filter( + (a) => + !childrenToPatch.some( + (b) => + a.index === b.index && + a.path === b.path && + a.caseSensitive === b.caseSensitive + ) + ); + + let newRoutes = convertRoutesToDataRoutes( + uniqueChildren, + mapRouteProperties, + [routeId || "_", "patch", String(childrenToPatch?.length || "0")], + manifest + ); + + childrenToPatch.push(...newRoutes); } /**