Remove middleware depth restrictions (#13172)

Author: brophdawg11

.changeset/popular-hats-love.md

@@ -0,0 +1,5 @@
+---
+"react-router": patch
+---
+
+[REMOVE] Remove middleware depth logic and always call middlware for all matches

integration/middleware-test.ts

@@ -2016,7 +2016,7 @@ test.describe("Middleware", () => {
       appFixture.close();
     });
 
-    test("only calls middleware as deep as needed for granular data requests", async ({
+    test("still calls middleware for all matches on granular data requests", async ({
       page,
     }) => {
       let fixture = await createFixture({
@@ -2101,7 +2101,7 @@ test.describe("Middleware", () => {
 
       (await page.$('a[href="/a/b"]'))?.click();
       await page.waitForSelector("[data-b]");
-      expect(await page.locator("[data-a]").textContent()).toBe("A: a");
+      expect(await page.locator("[data-a]").textContent()).toBe("A: a,b");
       expect(await page.locator("[data-b]").textContent()).toBe("B: a,b");
 
       appFixture.close();

packages/react-router/__tests__/router/context-middleware-test.ts

@@ -762,6 +762,10 @@ describe("context/middleware", () => {
           "parent action start",
           "child 1 start - throwing",
           "parent loader start",
+          "child 1 loader start",
+          "child 2 loader start",
+          "child 2 loader end",
+          "child 1 loader end",
           "parent loader end",
         ]);
         expect(router.state.loaderData).toMatchInlineSnapshot(`
@@ -897,6 +901,10 @@ describe("context/middleware", () => {
           "child 2 start",
           "child 2 end - throwing",
           "parent loader start",
+          "child 1 loader start",
+          "child 2 loader start",
+          "child 2 loader end",
+          "child 1 loader end",
           "parent loader end",
         ]);
         expect(router.state.loaderData).toEqual({

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

@@ -136,17 +136,6 @@ export function StreamTransfer({
   }
 }
 
-function middlewareErrorHandler(
-  e: MiddlewareError,
-  keyedResults: Record<string, DataStrategyResult>
-) {
-  // we caught an error running the middleware, copy that overtop any
-  // non-error result for the route
-  Object.assign(keyedResults, {
-    [e.routeId]: { type: "error", result: e.error },
-  });
-}
-
 export function getSingleFetchDataStrategy(
   manifest: AssetsManifest,
   routeModules: RouteModules,
@@ -161,17 +150,9 @@ export function getSingleFetchDataStrategy(
     if (request.method !== "GET") {
       return runMiddlewarePipeline(
         args,
-        matches.findIndex((m) => m.shouldLoad),
         false,
-        async (keyedResults) => {
-          let results = await singleFetchActionStrategy(
-            request,
-            matches,
-            basename
-          );
-          Object.assign(keyedResults, results);
-        },
-        middlewareErrorHandler
+        () => singleFetchActionStrategy(request, matches, basename),
+        (e) => ({ [e.routeId]: { type: "error", result: e.error } })
       ) as Promise<Record<string, DataStrategyResult>>;
     }
 
@@ -216,24 +197,11 @@ export function getSingleFetchDataStrategy(
           !manifest.routes[m.route.id]?.hasClientLoader
       );
       if (!foundRevalidatingServerLoader) {
-        // Skip single fetch and just call the loaders in parallel when this is
-        // a SPA mode navigation
-        let tailIdx = [...matches].reverse().findIndex((m) => m.shouldLoad);
-        let lowestLoadingIndex = tailIdx < 0 ? 0 : matches.length - 1 - tailIdx;
         return runMiddlewarePipeline(
           args,
-          lowestLoadingIndex,
           false,
-          async (keyedResults) => {
-            let results = await nonSsrStrategy(
-              manifest,
-              request,
-              matches,
-              basename
-            );
-            Object.assign(keyedResults, results);
-          },
-          middlewareErrorHandler
+          () => nonSsrStrategy(manifest, request, matches, basename),
+          (e) => ({ [e.routeId]: { type: "error", result: e.error } })
         ) as Promise<Record<string, DataStrategyResult>>;
       }
     }
@@ -242,47 +210,27 @@ export function getSingleFetchDataStrategy(
     if (fetcherKey) {
       return runMiddlewarePipeline(
         args,
-        matches.findIndex((m) => m.shouldLoad),
         false,
-        async (keyedResults) => {
-          let results = await singleFetchLoaderFetcherStrategy(
-            request,
-            matches,
-            basename
-          );
-          Object.assign(keyedResults, results);
-        },
-        middlewareErrorHandler
+        () => singleFetchLoaderFetcherStrategy(request, matches, basename),
+        (e) => ({ [e.routeId]: { type: "error", result: e.error } })
       ) as Promise<Record<string, DataStrategyResult>>;
     }
 
     // Navigational loads are more complex...
-
-    // Determine how deep to run middleware
-    let lowestLoadingIndex = getLowestLoadingIndex(
-      manifest,
-      routeModules,
-      getRouter(),
-      matches
-    );
-
     return runMiddlewarePipeline(
       args,
-      lowestLoadingIndex,
       false,
-      async (keyedResults) => {
-        let results = await singleFetchLoaderNavigationStrategy(
+      () =>
+        singleFetchLoaderNavigationStrategy(
           manifest,
           routeModules,
           ssr,
           getRouter(),
           request,
           matches,
           basename
-        );
-        Object.assign(keyedResults, results);
-      },
-      middlewareErrorHandler
+        ),
+      (e) => ({ [e.routeId]: { type: "error", result: e.error } })
     ) as Promise<Record<string, DataStrategyResult>>;
   };
 }
@@ -371,27 +319,6 @@ function isOptedOut(
   );
 }
 
-function getLowestLoadingIndex(
-  manifest: AssetsManifest,
-  routeModules: RouteModules,
-  router: DataRouter,
-  matches: DataStrategyFunctionArgs["matches"]
-) {
-  let tailIdx = [...matches]
-    .reverse()
-    .findIndex(
-      (m) =>
-        m.shouldLoad ||
-        !isOptedOut(
-          manifest.routes[m.route.id],
-          routeModules[m.route.id],
-          m,
-          router
-        )
-    );
-  return tailIdx < 0 ? 0 : matches.length - 1 - tailIdx;
-}
-
 // Loaders are trickier since we only want to hit the server once, so we
 // create a singular promise for all server-loader routes to latch onto.
 async function singleFetchLoaderNavigationStrategy(

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

@@ -3490,12 +3490,6 @@ export function createStaticHandler(
           "`requestContext` must bean instance of `unstable_RouterContextProvider`"
       );
       try {
-        // Run middleware as far deep as the deepest loader to be executed
-        let tailIdx = [...matches]
-          .reverse()
-          .findIndex((m) => !filterMatchesToLoad || filterMatchesToLoad(m));
-        let lowestLoadingIdx = tailIdx < 0 ? 0 : matches.length - 1 - tailIdx;
-
         let renderedStaticContext: StaticHandlerContext | undefined;
         let response = await runMiddlewarePipeline(
           {
@@ -3506,7 +3500,6 @@ export function createStaticHandler(
             // this to the proper type knowing it's not an `AppLoadContext`
             context: requestContext as unstable_RouterContextProvider,
           },
-          lowestLoadingIdx,
           true,
           async () => {
             let result = await queryImpl(
@@ -3700,7 +3693,6 @@ export function createStaticHandler(
           // this to the proper type knowing it's not an `AppLoadContext`
           context: requestContext as unstable_RouterContextProvider,
         },
-        matches.length - 1,
         true,
         async () => {
           let result = await queryImpl(
@@ -4940,91 +4932,65 @@ async function defaultDataStrategyWithMiddleware(
     return defaultDataStrategy(args);
   }
 
-  // Determine how far down we'll be loading so we only run middleware to that
-  // point.  This prevents us from calling middleware below an action error
-  // boundary below which we don't run loaders
-  let lastIndex = args.matches.length - 1;
-  for (let i = lastIndex; i >= 0; i--) {
-    if (args.matches[i].shouldLoad) {
-      lastIndex = i;
-      break;
-    }
-  }
-
-  let results = await runMiddlewarePipeline(
+  return runMiddlewarePipeline(
     args,
-    lastIndex,
     false,
-    async (keyedResults: Record<string, DataStrategyResult>) => {
-      Object.assign(keyedResults, await defaultDataStrategy(args));
-    },
-    (e, keyedResults) => {
-      // we caught an error running the middleware, copy that overtop any
-      // non-error result for the route
-      Object.assign(keyedResults, {
-        [e.routeId]: { type: "error", result: e.error },
-      });
-    }
-  );
-  return results as Record<string, DataStrategyResult>;
+    () => defaultDataStrategy(args),
+    (e) => ({ [e.routeId]: { type: "error", result: e.error } })
+  ) as Promise<Record<string, DataStrategyResult>>;
 }
 
 type MutableMiddlewareState = {
-  keyedResults: Record<string, DataStrategyResult>;
+  handlerResult: unknown;
   propagateResult: boolean;
 };
 
-export async function runMiddlewarePipeline(
-  {
-    request,
-    params,
-    context,
-    matches,
-  }: (
+export async function runMiddlewarePipeline<T extends boolean>(
+  args: (
     | LoaderFunctionArgs<unstable_RouterContextProvider>
     | ActionFunctionArgs<unstable_RouterContextProvider>
   ) & {
+    // Don't use `DataStrategyFunctionArgs` directly so we can we reduce these
+    // back from `DataStrategyMatch` to regular matches for use in the staticHandler
     matches: AgnosticDataRouteMatch[];
   },
-  lastIndex: number,
-  propagateResult: boolean,
-  handler: (results: Record<string, DataStrategyResult>) => unknown,
-  errorHandler: (
-    error: MiddlewareError,
-    results: Record<string, DataStrategyResult>
-  ) => unknown
+  propagateResult: T,
+  handler: () => T extends true
+    ? MaybePromise<Response>
+    : MaybePromise<Record<string, DataStrategyResult>>,
+  errorHandler: (error: MiddlewareError) => unknown
 ): Promise<unknown> {
+  let { matches, request, params, context } = args;
   let middlewareState: MutableMiddlewareState = {
-    keyedResults: {},
+    handlerResult: undefined,
     propagateResult,
   };
   try {
+    let tuples = matches.flatMap((m) =>
+      m.route.unstable_middleware
+        ? m.route.unstable_middleware.map((fn) => [m.route.id, fn])
+        : []
+    ) as [string, unstable_MiddlewareFunction][];
     let result = await callRouteMiddleware(
-      matches
-        .slice(0, lastIndex + 1)
-        .flatMap((m) =>
-          m.route.unstable_middleware
-            ? m.route.unstable_middleware.map((fn) => [m.route.id, fn])
-            : []
-        ) as [string, unstable_MiddlewareFunction][],
-      0,
       { request, params, context },
+      tuples,
       middlewareState,
       handler
     );
     return middlewareState.propagateResult
       ? result
-      : middlewareState.keyedResults;
+      : middlewareState.handlerResult;
   } catch (e) {
     if (!(e instanceof MiddlewareError)) {
       // This shouldn't happen?  This would have to come from a bug in our
       // library code...
       throw e;
     }
-    let result = await errorHandler(e, middlewareState.keyedResults);
-    return middlewareState.propagateResult
-      ? result
-      : middlewareState.keyedResults;
+    let result = await errorHandler(e);
+    if (propagateResult || !middlewareState.handlerResult) {
+      return result;
+    }
+    return Object.assign(middlewareState.handlerResult, result);
   }
 }
 
@@ -5038,13 +5004,13 @@ export class MiddlewareError {
 }
 
 async function callRouteMiddleware(
-  middlewares: [string, unstable_MiddlewareFunction][],
-  idx: number,
   args:
     | LoaderFunctionArgs<unstable_RouterContextProvider>
     | ActionFunctionArgs<unstable_RouterContextProvider>,
+  middlewares: [string, unstable_MiddlewareFunction][],
   middlewareState: MutableMiddlewareState,
-  handler: (r: Record<string, DataStrategyResult>) => void
+  handler: () => void,
+  idx = 0
 ): Promise<unknown> {
   let { request } = args;
   if (request.signal.aborted) {
@@ -5059,8 +5025,8 @@ async function callRouteMiddleware(
   let tuple = middlewares[idx];
   if (!tuple) {
     // We reached the end of our middlewares, call the handler
-    let result = await handler(middlewareState.keyedResults);
-    return result;
+    middlewareState.handlerResult = await handler();
+    return middlewareState.handlerResult;
   }
 
   let [routeId, middleware] = tuple;
@@ -5072,11 +5038,11 @@ async function callRouteMiddleware(
     }
     nextCalled = true;
     let result = await callRouteMiddleware(
-      middlewares,
-      idx + 1,
       args,
+      middlewares,
       middlewareState,
-      handler
+      handler,
+      idx + 1
     );
     if (middlewareState.propagateResult) {
       nextResult = result;