Handle middlewares returning responses without calling next

.changeset/grumpy-frogs-greet.md

@@ -0,0 +1,5 @@
+---
+"react-router": patch
+---
+
+[UNSTABLE] Propagate returned Response from server middleware if next wasn't called

.changeset/neat-dolls-join.md

@@ -0,0 +1,5 @@
+---
+"react-router": patch
+---
+
+[UNSTABLE] Allow server middlewares to return `data()` values which will be converted into a `Response`

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

@@ -1,6 +1,10 @@
 import { createMemoryHistory } from "../../lib/router/history";
 import type { Router, StaticHandlerContext } from "../../lib/router/router";
-import { createRouter, createStaticHandler } from "../../lib/router/router";
+import {
+  createRouter,
+  createStaticHandler,
+  isDataWithResponseInit,
+} from "../../lib/router/router";
 import type {
   DataStrategyResult,
   unstable_MiddlewareFunction,
@@ -10,6 +14,7 @@ import {
   unstable_createContext,
   redirect,
   unstable_RouterContextProvider,
+  data,
 } from "../../lib/router/utils";
 import { cleanup } from "./utils/data-router-setup";
 import { createFormData, tick } from "./utils/utils";
@@ -358,6 +363,9 @@ describe("context/middleware", () => {
 
       it("does not return result of middleware in client side routers", async () => {
         let values: unknown[] = [];
+        let consoleSpy = jest
+          .spyOn(console, "warn")
+          .mockImplementation(() => {});
         router = createRouter({
           history: createMemoryHistory(),
           routes: [
@@ -409,6 +417,8 @@ describe("context/middleware", () => {
           parent: "PARENT",
           child: [undefined, undefined, undefined, undefined],
         });
+
+        consoleSpy.mockRestore();
       });
 
       it("does not require that you call next()", async () => {
@@ -1631,6 +1641,62 @@ describe("context/middleware", () => {
       expect(res.headers.get("parent")).toEqual("yes");
     });
 
+    it("propagates a returned response if next isn't called", async () => {
+      let handler = createStaticHandler([
+        {
+          path: "/",
+        },
+        {
+          id: "parent",
+          path: "/parent",
+          unstable_middleware: [
+            async (_, next) => {
+              return new Response("test");
+            },
+          ],
+          loader() {
+            return "PARENT";
+          },
+        },
+      ]);
+
+      let res = (await handler.query(new Request("http://localhost/parent"), {
+        unstable_respond: respondWithJson,
+      })) as Response;
+      await expect(res.text()).resolves.toEqual("test");
+    });
+
+    it("propagates a returned data() response if next isn't called", async () => {
+      let handler = createStaticHandler([
+        {
+          path: "/",
+        },
+        {
+          id: "parent",
+          path: "/parent",
+          unstable_middleware: [
+            async (_, next) => {
+              let result = await next();
+              expect(isDataWithResponseInit(result)).toBe(true);
+              return result;
+            },
+            async (_, next) => {
+              return data("not found", { status: 404 });
+            },
+          ],
+          loader() {
+            return "PARENT";
+          },
+        },
+      ]);
+
+      let res = (await handler.query(new Request("http://localhost/parent"), {
+        unstable_respond: respondWithJson,
+      })) as Response;
+      expect(res.status).toBe(404);
+      await expect(res.text()).resolves.toEqual("not found");
+    });
+
     describe("ordering", () => {
       it("runs middleware sequentially before and after loaders", async () => {
         let handler = createStaticHandler([
@@ -2512,6 +2578,99 @@ describe("context/middleware", () => {
       expect(res.headers.get("child2")).toEqual("yes");
     });
 
+    it("propagates the response even if you call next and forget to return it", async () => {
+      let handler = createStaticHandler([
+        {
+          path: "/",
+        },
+        {
+          id: "parent",
+          path: "/parent",
+          unstable_middleware: [
+            async (_, next) => {
+              let res = (await next()) as Response;
+              res.headers.set("parent", "yes");
+            },
+          ],
+          loader() {
+            return new Response("PARENT");
+          },
+        },
+      ]);
+
+      let res = (await handler.queryRoute(
+        new Request("http://localhost/parent"),
+        {
+          unstable_respond: (v) => v,
+        },
+      )) as Response;
+
+      expect(await res.text()).toBe("PARENT");
+      expect(res.headers.get("parent")).toEqual("yes");
+    });
+
+    it("propagates a returned response if next isn't called", async () => {
+      let handler = createStaticHandler([
+        {
+          path: "/",
+        },
+        {
+          id: "parent",
+          path: "/parent",
+          unstable_middleware: [
+            async (_, next) => {
+              return new Response("test");
+            },
+          ],
+          loader() {
+            return "PARENT";
+          },
+        },
+      ]);
+
+      let res = (await handler.queryRoute(
+        new Request("http://localhost/parent"),
+        {
+          unstable_respond: (v) => v,
+        },
+      )) as Response;
+      await expect(res.text()).resolves.toEqual("test");
+    });
+
+    it("propagates a returned data() response if next isn't called", async () => {
+      let handler = createStaticHandler([
+        {
+          path: "/",
+        },
+        {
+          id: "parent",
+          path: "/parent",
+          unstable_middleware: [
+            async (_, next) => {
+              let result = await next();
+              expect(isDataWithResponseInit(result)).toBe(true);
+              return result;
+            },
+            async (_, next) => {
+              return data("not found", { status: 404 });
+            },
+          ],
+          loader() {
+            return "PARENT";
+          },
+        },
+      ]);
+
+      let res = (await handler.queryRoute(
+        new Request("http://localhost/parent"),
+        {
+          unstable_respond: (v) => v,
+        },
+      )) as Response;
+      expect(res.status).toBe(404);
+      await expect(res.text()).resolves.toEqual("not found");
+    });
+
     describe("ordering", () => {
       it("runs middleware sequentially before and after loaders", async () => {
         let handler = createStaticHandler([

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

@@ -3544,7 +3544,7 @@ export function createStaticHandler(
         ? requestContext
         : new unstable_RouterContextProvider();
 
-    let respondOrStreamStaticContext = (
+    let shortCircuitResult = (
       ctx: StaticHandlerContext,
     ): MaybePromise<Response> | StaticHandlerContext => {
       return stream
@@ -3574,7 +3574,7 @@ export function createStaticHandler(
         loaderHeaders: {},
         actionHeaders: {},
       };
-      return respondOrStreamStaticContext(staticContext);
+      return shortCircuitResult(staticContext);
     } else if (!matches) {
       let error = getInternalRouterError(404, { pathname: location.pathname });
       let { matches: notFoundMatches, route } =
@@ -3592,7 +3592,7 @@ export function createStaticHandler(
         loaderHeaders: {},
         actionHeaders: {},
       };
-      return respondOrStreamStaticContext(staticContext);
+      return shortCircuitResult(staticContext);
     }
 
     if (
@@ -3749,7 +3749,9 @@ export function createStaticHandler(
                   ? routeId
                   : findNearestBoundary(matches, routeId).route.id,
               );
-              return respondOrStreamStaticContext(staticContext);
+              return stream
+                ? stream(requestContext, () => Promise.resolve(staticContext))
+                : respond!(staticContext);
             } else {
               // We never even got to the handlers, so we've got no data -
               // just create an empty context reflecting the error.
@@ -3779,7 +3781,9 @@ export function createStaticHandler(
                 actionHeaders: {},
                 loaderHeaders: {},
               };
-              return respondOrStreamStaticContext(staticContext);
+              return stream
+                ? stream(requestContext, () => Promise.resolve(staticContext))
+                : respond!(staticContext);
             }
           },
         );
@@ -5543,7 +5547,12 @@ export async function runMiddlewarePipeline<T extends boolean>(
   handler: () => T extends true
     ? MaybePromise<Response>
     : MaybePromise<Record<string, DataStrategyResult>>,
-  errorHandler: (error: unknown, routeId: string) => unknown,
+  errorHandler: (
+    error: unknown,
+    routeId: string,
+  ) => T extends true
+    ? MaybePromise<Response>
+    : Record<string, DataStrategyResult>,
 ): Promise<unknown> {
   let { matches, request, params, context } = args;
   let middlewareState: MutableMiddlewareState = {
@@ -5562,7 +5571,26 @@ export async function runMiddlewarePipeline<T extends boolean>(
       middlewareState,
       handler,
     );
-    return propagateResult ? result : middlewareState.handlerResult;
+
+    if (propagateResult) {
+      invariant(
+        isResponse(result) || isDataWithResponseInit(result),
+        `Expected a Response to be returned from route middleware`,
+      );
+      // Upgrade returned data() calls to real Responses
+      if (isDataWithResponseInit(result)) {
+        return new Response(
+          typeof result.data === "string"
+            ? result.data
+            : JSON.stringify(result.data),
+          { ...result.init },
+        );
+      } else {
+        return result;
+      }
+    } else {
+      return middlewareState.handlerResult;
+    }
   } catch (e) {
     if (!middlewareState.middlewareError) {
       // This shouldn't happen?  This would have to come from a bug in our
@@ -5638,18 +5666,33 @@ async function callRouteMiddleware(
       },
       next,
     );
-    if (nextCalled) {
-      if (result === undefined) {
+
+    if (propagateResult) {
+      // On the server, handle calling next() if needed and returning the proper result
+      if (nextCalled) {
         // If they called next() but didn't return the response, we can bubble
-        // it for them. This lets folks do things like grab the response and
-        // add a header without then re-returning it
-        return nextResult;
-      } else {
+        // it for them. This allows some minor syntactic sugar (or forgetfulness)
+        // where you can grab the response to add a header without re-returning it
+        return typeof result === "undefined" ? nextResult : result;
+      } else if (isResponse(result) || isDataWithResponseInit(result)) {
+        // Use short circuit Response/data() values without having called next()
         return result;
+      } else {
+        // Otherwise call next() for them
+        nextResult = await next();
+        return nextResult;
       }
     } else {
-      result = await next();
-      return result;
+      // On the client, just call next if they didn't
+      if (typeof result !== "undefined") {
+        console.warn(
+          "client middlewares are not intended to return values, the value will be ignored",
+          result,
+        );
+      }
+      if (!nextCalled) {
+        await next();
+      }
     }
   } catch (error) {
     if (!middlewareState.middlewareError) {