Fix middleware error bubbling scenarios

Author: brophdawg11

.changeset/tough-dancers-own.md

@@ -0,0 +1,5 @@
+---
+"react-router": patch
+---
+
+UNSTABLE: Fix a few bugs with error bubbling in middleware use-cases

integration/middleware-test.ts

@@ -2140,7 +2140,7 @@ test.describe("Middleware", () => {
             "app/routes/_index.tsx": js`
               import { Link } from 'react-router'
               export default function Component({ loaderData }) {
-                return <Link to="/a/b">Link</Link>
+                return <Link to="/a/b/c/d">Link</Link>
               }
             `,
             "app/routes/a.tsx": js`
@@ -2158,7 +2158,7 @@ test.describe("Middleware", () => {
                 return null;
               }
               export default function Component() {
-                return <Outlet/>
+                return <Outlet />
               }
             `,
             "app/routes/a.b.c.tsx": js`
@@ -2192,6 +2192,109 @@ test.describe("Middleware", () => {
       expect(await page.locator("h1").textContent()).toBe("A Error Boundary");
       expect(await page.locator("pre").textContent()).toBe("broken!");
 
+      await app.goto("/");
+      await app.clickLink("/a/b/c/d");
+      expect(await page.locator("h1").textContent()).toBe("A Error Boundary");
+      expect(await page.locator("pre").textContent()).toBe("broken!");
+
+      appFixture.close();
+    });
+
+    test("bubbles errors on the way down up to the deepest error boundary when loaders aren't revalidating", async ({
+      page,
+    }) => {
+      let fixture = await createFixture(
+        {
+          files: {
+            "react-router.config.ts": reactRouterConfig({
+              middleware: true,
+            }),
+            "vite.config.ts": js`
+              import { defineConfig } from "vite";
+              import { reactRouter } from "@react-router/dev/vite";
+
+              export default defineConfig({
+                build: { manifest: true, minify: false },
+                plugins: [reactRouter()],
+              });
+            `,
+            "app/routes/_index.tsx": js`
+              import { Link } from 'react-router'
+              export default function Component({ loaderData }) {
+                return (
+                  <>
+                    <Link to="/a/b">/a/b</Link>
+                    <br/>
+                    <Link to="/a/b/c/d">/a/b/c/d</Link>
+                  </>
+                );
+              }
+            `,
+            "app/routes/a.tsx": js`
+              import { Outlet } from 'react-router'
+              export default function Component() {
+                return <Outlet/>
+              }
+              export function ErrorBoundary({ error }) {
+                return <><h1 data-error-a>A Error Boundary</h1><pre>{error.message}</pre></>
+              }
+            `,
+            "app/routes/a.b.tsx": js`
+              import { Link, Outlet } from 'react-router'
+              export function loader() {
+                return { message: "DATA" };
+              }
+              export default function Component({ loaderData }) {
+                return (
+                  <>
+                    <h2 data-ab>AB: {loaderData.message}</h2>
+                    <Link to="/a/b/c/d">/a/b/c/d</Link>
+                    <Outlet/>
+                  </>
+                );
+              }
+              export function shouldRevalidate() {
+                return false;
+              }
+            `,
+            "app/routes/a.b.c.tsx": js`
+              import { Outlet } from 'react-router'
+              export default function Component() {
+                return <Outlet/>
+              }
+              export function ErrorBoundary({ error }) {
+                return <><h1 data-error-c>C Error Boundary</h1><pre>{error.message}</pre></>
+              }
+            `,
+            "app/routes/a.b.c.d.tsx": js`
+              import { Outlet } from 'react-router'
+              export const unstable_middleware = [() => { throw new Error("broken!") }]
+              export const loader = () => null;
+              export default function Component() {
+                return <Outlet/>
+              }
+            `,
+          },
+        },
+        UNSAFE_ServerMode.Development
+      );
+
+      let appFixture = await createAppFixture(
+        fixture,
+        UNSAFE_ServerMode.Development
+      );
+
+      let app = new PlaywrightFixture(appFixture, page);
+      await app.goto("/");
+      await app.clickLink("/a/b");
+      await page.waitForSelector("[data-ab]");
+      expect(await page.locator("[data-ab]").textContent()).toBe("AB: DATA");
+
+      await app.clickLink("/a/b/c/d");
+      await page.waitForSelector("[data-error-c]");
+      expect(await page.locator("h1").textContent()).toBe("C Error Boundary");
+      expect(await page.locator("pre").textContent()).toBe("broken!");
+
       appFixture.close();
     });
 

packages/react-router/lib/dom/ssr/single-fetch.tsx

@@ -466,6 +466,20 @@ async function singleFetchLoaderNavigationStrategy(
 
   await resolvePromise;
 
+  // Capture any middleware errors from routes that weren't actually fetching
+  // data, these will be bubbled by the router in `processRouteLoaderData`
+  let fetchedData = await singleFetchDfd.promise;
+  if ("routes" in fetchedData) {
+    Object.entries(fetchedData.routes).forEach(([routeId, result]) => {
+      if ("error" in result && results[routeId]?.type !== "error") {
+        results[routeId] = {
+          type: "error",
+          result: result.error,
+        };
+      }
+    });
+  }
+
   return results;
 }
 
@@ -694,12 +708,14 @@ function unwrapSingleFetchResult(
   }
 
   let routeResult = result.routes[routeId];
-  if ("error" in routeResult) {
+  if (routeResult == null) {
+    throw new Error(`No result found for routeId "${routeId}"`);
+  } else if ("error" in routeResult) {
     throw routeResult.error;
   } else if ("data" in routeResult) {
     return routeResult.data;
   } else {
-    throw new Error(`No response found for routeId "${routeId}"`);
+    throw new Error(`Invalid response found for routeId "${routeId}"`);
   }
 }
 

packages/react-router/lib/router/router.ts

@@ -3614,28 +3614,50 @@ export function createStaticHandler(
                   dataRoutes,
                   renderedStaticContext,
                   error,
-                  findNearestBoundary(matches!, routeId).route.id
+                  skipLoaderErrorBubbling
+                    ? routeId
+                    : findNearestBoundary(matches, routeId).route.id
                 )
               );
             } else {
               // We never even got to the handlers, so we've got no data -
               // just create an empty context reflecting the error.
-              // Find the boundary at or above the highest loader.  We can't
-              // render any UI below there since we have no loader data available
-              let loaderIdx = matches!.findIndex((m) => m.route.loader);
-              let boundary =
-                loaderIdx >= 0
-                  ? findNearestBoundary(matches!, matches![loaderIdx].route.id)
-                  : findNearestBoundary(matches!);
+
+              let boundaryRouteId = skipLoaderErrorBubbling
+                ? routeId
+                : // Find the boundary at or above the source of the middleware
+                  // error or the highest loader. We can't render any UI below
+                  // the highest loader since we have no loader data available
+                  findNearestBoundary(
+                    matches,
+                    matches.find(
+                      (m) => m.route.id === routeId || m.route.loader
+                    )?.route.id || routeId
+                  ).route.id;
+
+              // If we errored in the top-down middleware, stub in `undefined`
+              // for all loaders the front end is expecting results for
+              let loaderData: RouterState["loaderData"] = {};
+              if (!isMutationMethod(request.method)) {
+                matches
+                  .filter((m) =>
+                    filterMatchesToLoad
+                      ? filterMatchesToLoad(m)
+                      : m.route.loader
+                  )
+                  .forEach((m) => {
+                    loaderData[m.route.id] = undefined;
+                  });
+              }
 
               return respond({
                 matches: matches!,
                 location,
                 basename,
-                loaderData: {},
+                loaderData,
                 actionData: null,
                 errors: {
-                  [boundary.route.id]: error,
+                  [boundaryRouteId]: error,
                 },
                 statusCode: isRouteErrorResponse(error) ? error.status : 500,
                 actionHeaders: {},
@@ -4219,6 +4241,7 @@ export function createStaticHandler(
       matches,
       results,
       pendingActionResult,
+      undefined,
       true,
       skipLoaderErrorBubbling
     );
@@ -5987,6 +6010,7 @@ function processRouteLoaderData(
   matches: AgnosticDataRouteMatch[],
   results: Record<string, DataResult>,
   pendingActionResult: PendingActionResult | undefined,
+  currentLoaderData?: RouterState["loaderData"],
   isStaticHandler = false,
   skipLoaderErrorBubbling = false
 ): {
@@ -6032,10 +6056,22 @@ function processRouteLoaderData(
       if (skipLoaderErrorBubbling) {
         errors[id] = error;
       } else {
-        // Look upwards from the matched route for the closest ancestor error
-        // boundary, defaulting to the root match.  Prefer higher error values
-        // if lower errors bubble to the same boundary
-        let boundaryMatch = findNearestBoundary(matches, id);
+        // Bubble the error to the proper error boundary by looking upwards from
+        // the highest route that defines a `loader` but doesn't currently have
+        // any `loaderData`.  This situation can happen if a middleware throws
+        // on the way down and thus a loader never executes for a given route.
+        // If such a route doesn't exist, then we just look upwards from the
+        // throwing route. Prefer higher error values if lower errors bubble to
+        // the same boundary
+        let highestLoaderRouteWithoutData = currentLoaderData
+          ? matches.find(
+              (m) => m.route.loader && !(m.route.id in currentLoaderData)
+            )
+          : undefined;
+        let boundaryMatch = findNearestBoundary(
+          matches,
+          highestLoaderRouteWithoutData?.route.id || id
+        );
         if (errors[boundaryMatch.route.id] == null) {
           errors[boundaryMatch.route.id] = error;
         }
@@ -6102,7 +6138,8 @@ function processLoaderData(
   let { loaderData, errors } = processRouteLoaderData(
     matches,
     results,
-    pendingActionResult
+    pendingActionResult,
+    state.loaderData
   );
 
   // Process results from our revalidating fetchers