Remove internal cache from patchRoutesOnNavigation (#12055)

Author: brophdawg11

.changeset/gold-frogs-pump.md

@@ -0,0 +1,10 @@
+---
+"@remix-run/router": patch
+---
+
+Remove internal cache to fix issues with interrupted `patchRoutesOnNavigation` calls
+
+- We used to cache in-progress calls to `patchRoutesOnNavigation` internally so that multiple navigations with the same start/end would only execute the function once and use the same promise
+- However, this approach was at odds with `patch` short circuiting if a navigation was interrupted (and the `request.signal` aborted) since the first invocation's `patch` would no-op
+- This cache also made some assumptions as to what a valid cache key might be - and is oblivious to any other application-state changes that may have occurred
+- So, the cache has been removed because in _most_ cases, repeated calls to something like `import()` for async routes will already be cached automatically - and if not it's easy enough for users to implement this cache in userland

docs/components/form.md

@@ -285,8 +285,6 @@ See also: [`<Link preventScrollReset>`][link-preventscrollreset]
 
 The `viewTransition` prop enables a [View Transition][view-transitions] for this navigation by wrapping the final state update in `document.startViewTransition()`. If you need to apply specific styles for this view transition, you will also need to leverage the [`useViewTransitionState()`][use-view-transition-state].
 
-<docs-warning>Please note that this API is marked unstable and may be subject to breaking changes without a major release</docs-warning>
-
 # Examples
 
 TODO: More examples

docs/components/link.md

@@ -185,8 +185,7 @@ If you need to apply specific styles for this view transition, you will also nee
 
 ```jsx
 function ImageLink(to) {
-  const isTransitioning =
-    useViewTransitionState(to);
+  const isTransitioning = useViewTransitionState(to);
   return (
     <Link to={to} viewTransition>
       <p
@@ -214,8 +213,6 @@ function ImageLink(to) {
 
 <docs-warning>`viewTransition` only works when using a data router, see [Picking a Router][picking-a-router]</docs-warning>
 
-<docs-warning>Please note that this API is marked unstable and may be subject to breaking changes without a major release</docs-warning>
-
 [link-native]: ./link-native
 [scrollrestoration]: ./scroll-restoration
 [history-replace-state]: https://developer.mozilla.org/en-US/docs/Web/API/History/replaceState

docs/components/nav-link.md

@@ -173,9 +173,5 @@ You may also use the `className`/`style` props or the render props passed to `ch
 </NavLink>
 ```
 
-<docs-warning>
-Please note that this API is marked unstable and may be subject to breaking changes without a major release.
-</docs-warning>
-
 [aria-current]: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-current
 [view-transitions]: https://developer.mozilla.org/en-US/docs/Web/API/View_Transitions_API

docs/hooks/use-fetcher.md

@@ -137,8 +137,6 @@ If you find yourself calling this function inside of click handlers, you can pro
 
 The `flushSync` option tells React Router DOM to wrap the initial state update for this `fetcher.load` in a [`ReactDOM.flushSync`][flush-sync] call instead of the default [`React.startTransition`][start-transition]. This allows you to perform synchronous DOM actions immediately after the update is flushed to the DOM.
 
-<docs-warning>Please note that this API is marked unstable and may be subject to breaking changes without a major release</docs-warning>
-
 ### `fetcher.submit()`
 
 The imperative version of `<fetcher.Form>`. If a user interaction should initiate the fetch, you should use `<fetcher.Form>`. But if you, the programmer are initiating the fetch (not in response to a user clicking a button, etc.), then use this function.

docs/hooks/use-navigate.md

@@ -120,16 +120,12 @@ The `flushSync` option tells React Router DOM to wrap the initial state update f
 
 <docs-warning>`flushSync` only works when using a data router, see [Picking a Router][picking-a-router]</docs-warning>
 
-<docs-warning>Please note that this API is marked unstable and may be subject to breaking changes without a major release</docs-warning>
-
 ## `options.viewTransition`
 
 The `viewTransition` option enables a [View Transition][view-transitions] for this navigation by wrapping the final state update in `document.startViewTransition()`. If you need to apply specific styles for this view transition, you will also need to leverage the [`useViewTransitionState()`][use-view-transition-state].
 
 <docs-warning>`viewTransition` only works when using a data router, see [Picking a Router][picking-a-router]</docs-warning>
 
-<docs-warning>Please note that this API is marked unstable and may be subject to breaking changes without a major release</docs-warning>
-
 [link]: ../components/link
 [redirect]: ../fetch/redirect
 [loaders]: ../route/loader

docs/hooks/use-submit.md

@@ -166,8 +166,6 @@ Because submissions are navigations, the options may also contain the other navi
 
 The `flushSync` option tells React Router DOM to wrap the initial state update for this submission in a [`ReactDOM.flushSync`][flush-sync] call instead of the default [`React.startTransition`][start-transition]. This allows you to perform synchronous DOM actions immediately after the update is flushed to the DOM.
 
-<docs-warning>Please note that this API is marked unstable and may be subject to breaking changes without a major release</docs-warning>
-
 [pickingarouter]: ../routers/picking-a-router
 [form]: ../components/form
 [flush-sync]: https://react.dev/reference/react-dom/flushSync

docs/routers/create-browser-router.md

@@ -188,8 +188,6 @@ const router = createBrowserRouter(
 
 <docs-warning>This is a low-level API intended for advanced use-cases. This overrides React Router's internal handling of `loader`/`action` execution, and if done incorrectly will break your app code. Please use with caution and perform the appropriate testing.</docs-warning>
 
-<docs-warning>This API is marked "unstable" so it is subject to breaking API changes in minor releases</docs-warning>
-
 By default, React Router is opinionated about how your data is loaded/submitted - and most notably, executes all of your loaders in parallel for optimal data fetching. While we think this is the right behavior for most use-cases, we realize that there is no "one size fits all" solution when it comes to data fetching for the wide landscape of application requirements.
 
 The `dataStrategy` option gives you full control over how your loaders and actions are executed and lays the foundation to build in more advanced APIs such as middleware, context, and caching layers. Over time, we expect that we'll leverage this API internally to bring more first class APIs to React Router, but until then (and beyond), this is your way to add more advanced functionality for your applications data needs.
@@ -436,11 +434,9 @@ let router = createBrowserRouter(routes, {
 
 ## `opts.patchRoutesOnNavigation`
 
-<docs-warning>This API is marked "unstable" so it is subject to breaking API changes in minor releases</docs-warning>
-
 By default, React Router wants you to provide a full route tree up front via `createBrowserRouter(routes)`. This allows React Router to perform synchronous route matching, execute loaders, and then render route components in the most optimistic manner without introducing waterfalls. The tradeoff is that your initial JS bundle is larger by definition - which may slow down application start-up times as your application grows.
 
-To combat this, we introduced [`route.lazy`][route-lazy] in [v6.9.0][6-9-0] which let's you lazily load the route _implementation_ (`loader`, `Component`, etc.) while still providing the route _definition_ aspects up front (`path`, `index`, etc.). This is a good middle ground because React Router still knows about your routes up front and can perform synchronous route matching, but then delay loading any of the route implementation aspects until the route is actually navigated to.
+To combat this, we introduced [`route.lazy`][route-lazy] in [v6.9.0][6-9-0] which let's you lazily load the route _implementation_ (`loader`, `Component`, etc.) while still providing the route _definition_ aspects up front (`path`, `index`, etc.). This is a good middle ground because React Router still knows about your route definitions (the lightweight part) up front and can perform synchronous route matching, but then delay loading any of the route implementation aspects (the heavier part) until the route is actually navigated to.
 
 In some cases, even this doesn't go far enough. For very large applications, providing all route definitions up front can be prohibitively expensive. Additionally, it might not even be possible to provide all route definitions up front in certain Micro-Frontend or Module-Federation architectures.
 
@@ -489,7 +485,7 @@ const router = createBrowserRouter(
 );
 ```
 
-In the above example, if the user clicks a link to `/a`, React Router won't be able to match it initially and will call `patchRoutesOnNavigation` with `/a` and a `matches` array containing the root route match. By calling `patch`, the `a` route will be added to the route tree and React Router will perform matching again. This time it will successfully match the `/a` path and the navigation will complete successfully.
+In the above example, if the user clicks a link to `/a`, React Router won't match any routes initially and will call `patchRoutesOnNavigation` with a `path = "/a"` and a `matches` array containing the root route match. By calling `patch('root', [route])`, the new route will be added to the route tree as a child of the `root` route and React Router will perform matching on the updated routes. This time it will successfully match the `/a` path and the navigation will complete successfully.
 
 **Patching new root-level routes**
 
@@ -544,6 +540,8 @@ let router = createBrowserRouter(
 );
 ```
 
+<docs-info>If in-progress execution of `patchRoutesOnNavigation` is interrupted by a subsequent navigation, then any remaining `patch` calls in the interrupted execution will not update the route tree because the operation was cancelled.</docs-info>
+
 **Co-locating route discovery with route definition**
 
 If you don't wish to perform your own pseudo-matching, you can leverage the partial `matches` array and the `handle` field on a route to keep the children definitions co-located:

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

@@ -1,5 +1,5 @@
 import type { AgnosticDataRouteObject, Router } from "../index";
-import { createMemoryHistory, createRouter } from "../index";
+import { IDLE_NAVIGATION, createMemoryHistory, createRouter } from "../index";
 import { ErrorResponseImpl } from "../utils";
 import { createDeferred, createFormData, tick } from "./utils/utils";
 
@@ -269,7 +269,7 @@ describe("Lazy Route Discovery (Fog of War)", () => {
     ]);
   });
 
-  it("reuses promises", async () => {
+  it("does not reuse former calls to patchRoutes on interruptions", async () => {
     let aDfd = createDeferred<AgnosticDataRouteObject[]>();
     let calls: string[][] = [];
     router = createRouter({
@@ -305,8 +305,10 @@ describe("Lazy Route Discovery (Fog of War)", () => {
     expect(router.state).toMatchObject({
       navigation: { state: "submitting", location: { pathname: "/a/b" } },
     });
-    // Didn't call again for the same path
-    expect(calls).toEqual([["/a/b", "a"]]);
+    expect(calls).toEqual([
+      ["/a/b", "a"],
+      ["/a/b", "a"],
+    ]);
 
     aDfd.resolve([
       {
@@ -321,10 +323,71 @@ describe("Lazy Route Discovery (Fog of War)", () => {
       navigation: { state: "idle" },
       location: { pathname: "/a/b" },
     });
-    expect(calls).toEqual([["/a/b", "a"]]);
+    expect(calls).toEqual([
+      ["/a/b", "a"],
+      ["/a/b", "a"],
+    ]);
+  });
+
+  it("handles interruptions when navigating to the same route", async () => {
+    let dfd1 = createDeferred<AgnosticDataRouteObject[]>();
+    let dfd2 = createDeferred<AgnosticDataRouteObject[]>();
+    let called = false;
+    router = createRouter({
+      history: createMemoryHistory(),
+      routes: [
+        {
+          path: "/",
+        },
+      ],
+      async patchRoutesOnNavigation({ patch }) {
+        if (!called) {
+          called = true;
+          patch(null, await dfd1.promise);
+        } else {
+          patch(null, await dfd2.promise);
+        }
+      },
+    });
+
+    router.navigate("/a");
+    await tick();
+    expect(router.state).toMatchObject({
+      navigation: { state: "loading", location: { pathname: "/a" } },
+    });
+
+    router.navigate("/a");
+    await tick();
+    expect(router.state).toMatchObject({
+      navigation: { state: "loading", location: { pathname: "/a" } },
+    });
+
+    dfd1.resolve([
+      {
+        id: "a1",
+        path: "/a",
+      },
+    ]);
+    await tick();
+    expect(router.state).toMatchObject({
+      navigation: { state: "loading", location: { pathname: "/a" } },
+    });
+
+    dfd2.resolve([
+      {
+        id: "a2",
+        path: "/a",
+      },
+    ]);
+    await tick();
+    expect(router.state).toMatchObject({
+      location: { pathname: "/a" },
+      navigation: IDLE_NAVIGATION,
+      matches: [{ route: { id: "a2" } }],
+    });
   });
 
-  it("handles interruptions", async () => {
+  it("handles interruptions when navigating to a new route", async () => {
     let aDfd = createDeferred<AgnosticDataRouteObject[]>();
     let bDfd = createDeferred<AgnosticDataRouteObject[]>();
     router = createRouter({

packages/router/router.ts

@@ -3295,21 +3295,30 @@ export function createRouter(init: RouterInit): Router {
     pathname: string,
     signal: AbortSignal
   ): Promise<DiscoverRoutesResult> {
+    if (!patchRoutesOnNavigationImpl) {
+      return { type: "success", matches };
+    }
+
     let partialMatches: AgnosticDataRouteMatch[] | null = matches;
     while (true) {
       let isNonHMR = inFlightDataRoutes == null;
       let routesToUse = inFlightDataRoutes || dataRoutes;
+      let localManifest = manifest;
       try {
-        await loadLazyRouteChildren(
-          patchRoutesOnNavigationImpl!,
-          pathname,
-          partialMatches,
-          routesToUse,
-          manifest,
-          mapRouteProperties,
-          pendingPatchRoutes,
-          signal
-        );
+        await patchRoutesOnNavigationImpl({
+          path: pathname,
+          matches: partialMatches,
+          patch: (routeId, children) => {
+            if (signal.aborted) return;
+            patchRoutesImpl(
+              routeId,
+              children,
+              routesToUse,
+              localManifest,
+              mapRouteProperties
+            );
+          },
+        });
       } catch (e) {
         return { type: "error", error: e, partialMatches };
       } finally {
@@ -3319,7 +3328,7 @@ export function createRouter(init: RouterInit): Router {
         // trigger a re-run of memoized `router.routes` dependencies.
         // HMR will already update the identity and reflow when it lands
         // `inFlightDataRoutes` in `completeNavigation`
-        if (isNonHMR) {
+        if (isNonHMR && !signal.aborted) {
           dataRoutes = [...dataRoutes];
         }
       }
@@ -4601,53 +4610,6 @@ function shouldRevalidateLoader(
   return arg.defaultShouldRevalidate;
 }
 
-/**
- * Idempotent utility to execute patchRoutesOnNavigation() to lazily load route
- * definitions and update the routes/routeManifest
- */
-async function loadLazyRouteChildren(
-  patchRoutesOnNavigationImpl: AgnosticPatchRoutesOnNavigationFunction,
-  path: string,
-  matches: AgnosticDataRouteMatch[],
-  routes: AgnosticDataRouteObject[],
-  manifest: RouteManifest,
-  mapRouteProperties: MapRoutePropertiesFunction,
-  pendingRouteChildren: Map<
-    string,
-    ReturnType<typeof patchRoutesOnNavigationImpl>
-  >,
-  signal: AbortSignal
-) {
-  let key = [path, ...matches.map((m) => m.route.id)].join("-");
-  try {
-    let pending = pendingRouteChildren.get(key);
-    if (!pending) {
-      pending = patchRoutesOnNavigationImpl({
-        path,
-        matches,
-        patch: (routeId, children) => {
-          if (!signal.aborted) {
-            patchRoutesImpl(
-              routeId,
-              children,
-              routes,
-              manifest,
-              mapRouteProperties
-            );
-          }
-        },
-      });
-      pendingRouteChildren.set(key, pending);
-    }
-
-    if (pending && isPromise<AgnosticRouteObject[]>(pending)) {
-      await pending;
-    }
-  } finally {
-    pendingRouteChildren.delete(key);
-  }
-}
-
 function patchRoutesImpl(
   routeId: string | null,
   children: AgnosticRouteObject[],