Revert "Remove middleware depth restrictions (#13172)"

This reverts commit fb014ce.

Author: brophdawg11

.changeset/popular-hats-love.md

@@ -2016,7 +2016,7 @@ test.describe("Middleware", () => {
       appFixture.close();
     });
 
-    test("still calls middleware for all matches on granular data requests", async ({
+    test("only calls middleware as deep as needed for 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,b");
+      expect(await page.locator("[data-a]").textContent()).toBe("A: a");
       expect(await page.locator("[data-b]").textContent()).toBe("B: a,b");
 
       appFixture.close();

integration/middleware-test.ts

@@ -762,10 +762,6 @@ 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(`
@@ -901,10 +897,6 @@ 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/__tests__/router/context-middleware-test.ts

@@ -136,6 +136,17 @@ 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,
@@ -150,9 +161,17 @@ export function getSingleFetchDataStrategy(
     if (request.method !== "GET") {
       return runMiddlewarePipeline(
         args,
+        matches.findIndex((m) => m.shouldLoad),
         false,
-        () => singleFetchActionStrategy(request, matches, basename),
-        (e) => ({ [e.routeId]: { type: "error", result: e.error } })
+        async (keyedResults) => {
+          let results = await singleFetchActionStrategy(
+            request,
+            matches,
+            basename
+          );
+          Object.assign(keyedResults, results);
+        },
+        middlewareErrorHandler
       ) as Promise<Record<string, DataStrategyResult>>;
     }
 
@@ -197,11 +216,24 @@ 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,
-          () => nonSsrStrategy(manifest, request, matches, basename),
-          (e) => ({ [e.routeId]: { type: "error", result: e.error } })
+          async (keyedResults) => {
+            let results = await nonSsrStrategy(
+              manifest,
+              request,
+              matches,
+              basename
+            );
+            Object.assign(keyedResults, results);
+          },
+          middlewareErrorHandler
         ) as Promise<Record<string, DataStrategyResult>>;
       }
     }
@@ -210,27 +242,47 @@ export function getSingleFetchDataStrategy(
     if (fetcherKey) {
       return runMiddlewarePipeline(
         args,
+        matches.findIndex((m) => m.shouldLoad),
         false,
-        () => singleFetchLoaderFetcherStrategy(request, matches, basename),
-        (e) => ({ [e.routeId]: { type: "error", result: e.error } })
+        async (keyedResults) => {
+          let results = await singleFetchLoaderFetcherStrategy(
+            request,
+            matches,
+            basename
+          );
+          Object.assign(keyedResults, results);
+        },
+        middlewareErrorHandler
       ) 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,
-      () =>
-        singleFetchLoaderNavigationStrategy(
+      async (keyedResults) => {
+        let results = await singleFetchLoaderNavigationStrategy(
           manifest,
           routeModules,
           ssr,
           getRouter(),
           request,
           matches,
           basename
-        ),
-      (e) => ({ [e.routeId]: { type: "error", result: e.error } })
+        );
+        Object.assign(keyedResults, results);
+      },
+      middlewareErrorHandler
     ) as Promise<Record<string, DataStrategyResult>>;
   };
 }
@@ -319,6 +371,27 @@ 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/dom/ssr/single-fetch.tsx

@@ -3490,6 +3490,12 @@ 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(
           {
@@ -3500,6 +3506,7 @@ 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(
@@ -3693,6 +3700,7 @@ 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(
@@ -4932,65 +4940,91 @@ async function defaultDataStrategyWithMiddleware(
     return defaultDataStrategy(args);
   }
 
-  return runMiddlewarePipeline(
+  // 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(
     args,
+    lastIndex,
     false,
-    () => defaultDataStrategy(args),
-    (e) => ({ [e.routeId]: { type: "error", result: e.error } })
-  ) as Promise<Record<string, DataStrategyResult>>;
+    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>;
 }
 
 type MutableMiddlewareState = {
-  handlerResult: unknown;
+  keyedResults: Record<string, DataStrategyResult>;
   propagateResult: boolean;
 };
 
-export async function runMiddlewarePipeline<T extends boolean>(
-  args: (
+export async function runMiddlewarePipeline(
+  {
+    request,
+    params,
+    context,
+    matches,
+  }: (
     | 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[];
   },
-  propagateResult: T,
-  handler: () => T extends true
-    ? MaybePromise<Response>
-    : MaybePromise<Record<string, DataStrategyResult>>,
-  errorHandler: (error: MiddlewareError) => unknown
+  lastIndex: number,
+  propagateResult: boolean,
+  handler: (results: Record<string, DataStrategyResult>) => unknown,
+  errorHandler: (
+    error: MiddlewareError,
+    results: Record<string, DataStrategyResult>
+  ) => unknown
 ): Promise<unknown> {
-  let { matches, request, params, context } = args;
   let middlewareState: MutableMiddlewareState = {
-    handlerResult: undefined,
+    keyedResults: {},
     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.handlerResult;
+      : middlewareState.keyedResults;
   } 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);
-    if (propagateResult || !middlewareState.handlerResult) {
-      return result;
-    }
-    return Object.assign(middlewareState.handlerResult, result);
+    let result = await errorHandler(e, middlewareState.keyedResults);
+    return middlewareState.propagateResult
+      ? result
+      : middlewareState.keyedResults;
   }
 }
 
@@ -5004,13 +5038,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: () => void,
-  idx = 0
+  handler: (r: Record<string, DataStrategyResult>) => void
 ): Promise<unknown> {
   let { request } = args;
   if (request.signal.aborted) {
@@ -5025,8 +5059,8 @@ async function callRouteMiddleware(
   let tuple = middlewares[idx];
   if (!tuple) {
     // We reached the end of our middlewares, call the handler
-    middlewareState.handlerResult = await handler();
-    return middlewareState.handlerResult;
+    let result = await handler(middlewareState.keyedResults);
+    return result;
   }
 
   let [routeId, middleware] = tuple;
@@ -5038,11 +5072,11 @@ async function callRouteMiddleware(
     }
     nextCalled = true;
     let result = await callRouteMiddleware(
-      args,
       middlewares,
+      idx + 1,
+      args,
       middlewareState,
-      handler,
-      idx + 1
+      handler
     );
     if (middlewareState.propagateResult) {
       nextResult = result;