Remove internal discoverRoutes queue and make patch idempotent (#11977)

Author: brophdawg11

.changeset/unlucky-keys-collect.md

@@ -0,0 +1,5 @@
+---
+"@remix-run/router": patch
+---
+
+Remove internal `discoveredRoutes` FIFO queue from `unstable_patchRoutesOnNavigation`

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`.

packages/router/__tests__/lazy-discovery-test.ts

@@ -1259,134 +1259,75 @@ 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(),
       routes: [
         {
           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", () => {

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<RouterSubscriber>();
-  // FIFO queue of previously discovered routes to prevent re-calling on
-  // subsequent navigations to the same path
-  let discoveredRoutesMaxSize = 1000;
-  let discoveredRoutes = new Set<string>();
   // Externally-provided object to hold scroll restoration locations during routing
   let savedScrollPositions: Record<string, number> | 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<AgnosticDataRouteObject>(
           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,22 +3340,13 @@ export function createRouter(init: RouterInit): Router {
             (m, i) => m.route.id === newPartialMatches![i].route.id
           ))
       ) {
-        addToFifoQueue(pathname, discoveredRoutes);
         return { type: "success", matches: null };
       }
 
       partialMatches = newPartialMatches;
     }
   }
 
-  function addToFifoQueue(path: string, queue: Set<string>) {
-    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);
 }
 
 /**