From bd4ac8ebec461e608d2aa411d1b88700d4519d75 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 13 Aug 2024 15:10:12 -0400 Subject: [PATCH] Call patchRoutesOnMiss when matching slug routes in case there exist higher scoring static routes (#11883) --- .changeset/cyan-bobcats-notice.md | 7 + .../router/__tests__/lazy-discovery-test.ts | 326 ++++++++++++++++++ packages/router/router.ts | 74 ++-- 3 files changed, 365 insertions(+), 42 deletions(-) create mode 100644 .changeset/cyan-bobcats-notice.md diff --git a/.changeset/cyan-bobcats-notice.md b/.changeset/cyan-bobcats-notice.md new file mode 100644 index 0000000000..cb696a9fe8 --- /dev/null +++ b/.changeset/cyan-bobcats-notice.md @@ -0,0 +1,7 @@ +--- +"@remix-run/router": patch +--- + +Fog of War: Update `unstable_patchRoutesOnMiss` logic so that we call the method when we match routes with dynamic param or splat segments in case there exists a higher-scoring static route that we've not yet discovered. + +- We also now leverage an internal FIFO queue of previous paths we've already called `unstable_patchRouteOnMiss` against so that we don't re-call on subsequent navigations to the same path diff --git a/packages/router/__tests__/lazy-discovery-test.ts b/packages/router/__tests__/lazy-discovery-test.ts index 68f6d41f7c..808f498873 100644 --- a/packages/router/__tests__/lazy-discovery-test.ts +++ b/packages/router/__tests__/lazy-discovery-test.ts @@ -507,6 +507,121 @@ describe("Lazy Route Discovery (Fog of War)", () => { ]); }); + it("de-prioritizes dynamic param routes in favor of looking for better async matches", async () => { + router = createRouter({ + history: createMemoryHistory(), + routes: [ + { + path: "/", + }, + { + id: "slug", + path: "/:slug", + }, + ], + async unstable_patchRoutesOnMiss({ patch }) { + await tick(); + patch(null, [ + { + id: "static", + path: "/static", + }, + ]); + }, + }); + + await router.navigate("/static"); + expect(router.state.location.pathname).toBe("/static"); + expect(router.state.matches.map((m) => m.route.id)).toEqual(["static"]); + }); + + it("de-prioritizes dynamic param routes in favor of looking for better async matches (product/:slug)", async () => { + router = createRouter({ + history: createMemoryHistory(), + routes: [ + { + path: "/", + }, + { + id: "slug", + path: "/product/:slug", + }, + ], + async unstable_patchRoutesOnMiss({ patch }) { + await tick(); + patch(null, [ + { + id: "static", + path: "/product/static", + }, + ]); + }, + }); + + await router.navigate("/product/static"); + expect(router.state.location.pathname).toBe("/product/static"); + expect(router.state.matches.map((m) => m.route.id)).toEqual(["static"]); + }); + + it("de-prioritizes dynamic param routes in favor of looking for better async matches (child route)", async () => { + router = createRouter({ + history: createMemoryHistory(), + routes: [ + { + path: "/", + }, + { + id: "product", + path: "/product", + children: [ + { + id: "slug", + path: ":slug", + }, + ], + }, + ], + async unstable_patchRoutesOnMiss({ patch }) { + await tick(); + patch("product", [ + { + id: "static", + path: "static", + }, + ]); + }, + }); + + await router.navigate("/product/static"); + expect(router.state.location.pathname).toBe("/product/static"); + expect(router.state.matches.map((m) => m.route.id)).toEqual([ + "product", + "static", + ]); + }); + + it("matches dynamic params when other paths don't pan out", async () => { + router = createRouter({ + history: createMemoryHistory(), + routes: [ + { + path: "/", + }, + { + id: "slug", + path: "/:slug", + }, + ], + async unstable_patchRoutesOnMiss({ matches, patch }) { + await tick(); + }, + }); + + await router.navigate("/a"); + expect(router.state.location.pathname).toBe("/a"); + expect(router.state.matches.map((m) => m.route.id)).toEqual(["slug"]); + }); + it("de-prioritizes splat routes in favor of looking for better async matches", async () => { router = createRouter({ history: createMemoryHistory(), @@ -569,6 +684,43 @@ describe("Lazy Route Discovery (Fog of War)", () => { expect(router.state.matches.map((m) => m.route.id)).toEqual(["static"]); }); + it("de-prioritizes splat routes in favor of looking for better async matches (child route)", async () => { + router = createRouter({ + history: createMemoryHistory(), + routes: [ + { + path: "/", + }, + { + id: "product", + path: "/product", + children: [ + { + id: "splat", + path: "*", + }, + ], + }, + ], + async unstable_patchRoutesOnMiss({ patch }) { + await tick(); + patch("product", [ + { + id: "static", + path: "static", + }, + ]); + }, + }); + + await router.navigate("/product/static"); + expect(router.state.location.pathname).toBe("/product/static"); + expect(router.state.matches.map((m) => m.route.id)).toEqual([ + "product", + "static", + ]); + }); + it("matches splats when other paths don't pan out", async () => { router = createRouter({ history: createMemoryHistory(), @@ -603,6 +755,50 @@ describe("Lazy Route Discovery (Fog of War)", () => { expect(router.state.matches.map((m) => m.route.id)).toEqual(["splat"]); }); + it("recurses unstable_patchRoutesOnMiss until a match is found", async () => { + let count = 0; + router = createRouter({ + history: createMemoryHistory(), + routes: [ + { + path: "/", + }, + { + id: "a", + path: "a", + }, + ], + async unstable_patchRoutesOnMiss({ matches, patch }) { + await tick(); + count++; + if (last(matches).route.id === "a") { + patch("a", [ + { + id: "b", + path: "b", + }, + ]); + } else if (last(matches).route.id === "b") { + patch("b", [ + { + id: "c", + path: "c", + }, + ]); + } + }, + }); + + await router.navigate("/a/b/c"); + expect(router.state.location.pathname).toBe("/a/b/c"); + expect(router.state.matches.map((m) => m.route.id)).toEqual([ + "a", + "b", + "c", + ]); + expect(count).toBe(2); + }); + it("discovers routes during initial hydration", async () => { let childrenDfd = createDeferred(); let loaderDfd = createDeferred(); @@ -1063,6 +1259,136 @@ describe("Lazy Route Discovery (Fog of War)", () => { unsubscribe(); }); + it('does not re-call for previously called "good" paths', async () => { + let count = 0; + router = createRouter({ + history: createMemoryHistory(), + routes: [ + { + path: "/", + }, + { + id: "param", + path: ":param", + }, + ], + async unstable_patchRoutesOnMiss() { + count++; + 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"); + 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: [ + { + id: "index", + path: "/", + }, + { + id: "static", + path: "static", + }, + ], + async unstable_patchRoutesOnMiss() { + 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: [ + { + path: "/", + }, + { + id: "param", + path: ":param", + }, + ], + async unstable_patchRoutesOnMiss() { + 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 patchRoutesOnMiss 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 patchRoutesOnMiss and evict the first item + await router.navigate(`/path-1001`); + expect(count).toBe(1001); + expect(router.state.location.pathname).toBe(`/path-1001`); + + // Call patchRoutesOnMiss since this item was evicted + await router.navigate(`/path-1`); + expect(count).toBe(1002); + expect(router.state.location.pathname).toBe(`/path-1`); + }); + describe("errors", () => { it("lazy 404s (GET navigation)", async () => { let childrenDfd = createDeferred(); diff --git a/packages/router/router.ts b/packages/router/router.ts index 05d19c4ccf..dde964f58d 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -814,6 +814,10 @@ 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 @@ -3183,6 +3187,13 @@ export function createRouter(init: RouterInit): Router { pathname: string ): { active: boolean; matches: AgnosticDataRouteMatch[] | null } { if (patchRoutesOnMissImpl) { + // 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, @@ -3193,14 +3204,10 @@ export function createRouter(init: RouterInit): Router { return { active: true, matches: fogMatches || [] }; } else { - let leafRoute = matches[matches.length - 1].route; - if ( - leafRoute.path && - (leafRoute.path === "*" || leafRoute.path.endsWith("/*")) - ) { - // If we matched a splat, it might only be because we haven't yet fetched - // the children that would match with a higher score, so let's fetch - // around and find out + if (Object.keys(matches[0].params).length > 0) { + // If we matched a dynamic param or a splat, it might only be because + // we haven't yet discovered other routes that would match with a + // higher score. Call patchRoutesOnMiss just to be sure let partialMatches = matchRoutesImpl( routesToUse, pathname, @@ -3236,10 +3243,6 @@ export function createRouter(init: RouterInit): Router { signal: AbortSignal ): Promise { let partialMatches: AgnosticDataRouteMatch[] | null = matches; - let route = - partialMatches.length > 0 - ? partialMatches[partialMatches.length - 1].route - : null; while (true) { let isNonHMR = inFlightDataRoutes == null; let routesToUse = inFlightDataRoutes || dataRoutes; @@ -3273,26 +3276,9 @@ export function createRouter(init: RouterInit): Router { } let newMatches = matchRoutes(routesToUse, pathname, basename); - let matchedSplat = false; if (newMatches) { - let leafRoute = newMatches[newMatches.length - 1].route; - - if (leafRoute.index) { - // If we found an index route, we can stop - return { type: "success", matches: newMatches }; - } - - if (leafRoute.path && leafRoute.path.length > 0) { - if (leafRoute.path === "*") { - // If we found a splat route, we can't be sure there's not a - // higher-scoring route down some partial matches trail so we need - // to check that out - matchedSplat = true; - } else { - // If we found a non-splat route, we can stop - return { type: "success", matches: newMatches }; - } - } + addToFifoQueue(pathname, discoveredRoutes); + return { type: "success", matches: newMatches }; } let newPartialMatches = matchRoutesImpl( @@ -3302,26 +3288,30 @@ export function createRouter(init: RouterInit): Router { true ); - // If we are no longer partially matching anything, this was either a - // legit splat match above, or it's a 404. Also avoid loops if the - // second pass results in the same partial matches + // Avoid loops if the second pass results in the same partial matches if ( !newPartialMatches || - partialMatches.map((m) => m.route.id).join("-") === - newPartialMatches.map((m) => m.route.id).join("-") + (partialMatches.length === newPartialMatches.length && + partialMatches.every( + (m, i) => m.route.id === newPartialMatches![i].route.id + )) ) { - return { type: "success", matches: matchedSplat ? newMatches : null }; + addToFifoQueue(pathname, discoveredRoutes); + return { type: "success", matches: null }; } partialMatches = newPartialMatches; - route = partialMatches[partialMatches.length - 1].route; - if (route.path === "*") { - // The splat is still our most accurate partial, so run with it - return { type: "success", matches: partialMatches }; - } } } + 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(