Skip to content

Commit 413f894

Browse files
brophdawg11brophdawg11
authored
Handle middlewares returning responses without calling next (#14093)
* Propagate returned Response from server middleware if next wasn't called * Allow middleware to return data() responses
1 parent beeb329 commit 413f894

File tree

4 files changed

+228
-16
lines changed

4 files changed

+228
-16
lines changed

‎.changeset/grumpy-frogs-greet.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"react-router": patch
3+
---
4+
5+
[UNSTABLE] Propagate returned Response from server middleware if next wasn't called

‎.changeset/neat-dolls-join.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"react-router": patch
3+
---
4+
5+
[UNSTABLE] Allow server middlewares to return `data()` values which will be converted into a `Response`

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

Lines changed: 160 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
import { createMemoryHistory } from "../../lib/router/history";
22
import type { Router, StaticHandlerContext } from "../../lib/router/router";
3-
import { createRouter, createStaticHandler } from "../../lib/router/router";
3+
import {
4+
createRouter,
5+
createStaticHandler,
6+
isDataWithResponseInit,
7+
} from "../../lib/router/router";
48
import type {
59
DataStrategyResult,
610
unstable_MiddlewareFunction,
@@ -10,6 +14,7 @@ import {
1014
unstable_createContext,
1115
redirect,
1216
unstable_RouterContextProvider,
17+
data,
1318
} from "../../lib/router/utils";
1419
import { cleanup } from "./utils/data-router-setup";
1520
import { createFormData, tick } from "./utils/utils";
@@ -358,6 +363,9 @@ describe("context/middleware", () => {
358363

359364
it("does not return result of middleware in client side routers", async () => {
360365
let values: unknown[] = [];
366+
let consoleSpy = jest
367+
.spyOn(console, "warn")
368+
.mockImplementation(() => {});
361369
router = createRouter({
362370
history: createMemoryHistory(),
363371
routes: [
@@ -409,6 +417,8 @@ describe("context/middleware", () => {
409417
parent: "PARENT",
410418
child: [undefined, undefined, undefined, undefined],
411419
});
420+
421+
consoleSpy.mockRestore();
412422
});
413423

414424
it("does not require that you call next()", async () => {
@@ -1631,6 +1641,62 @@ describe("context/middleware", () => {
16311641
expect(res.headers.get("parent")).toEqual("yes");
16321642
});
16331643

1644+
it("propagates a returned response if next isn't called", async () => {
1645+
let handler = createStaticHandler([
1646+
{
1647+
path: "/",
1648+
},
1649+
{
1650+
id: "parent",
1651+
path: "/parent",
1652+
unstable_middleware: [
1653+
async (_, next) => {
1654+
return new Response("test");
1655+
},
1656+
],
1657+
loader() {
1658+
return "PARENT";
1659+
},
1660+
},
1661+
]);
1662+
1663+
let res = (await handler.query(new Request("http://localhost/parent"), {
1664+
unstable_respond: respondWithJson,
1665+
})) as Response;
1666+
await expect(res.text()).resolves.toEqual("test");
1667+
});
1668+
1669+
it("propagates a returned data() response if next isn't called", async () => {
1670+
let handler = createStaticHandler([
1671+
{
1672+
path: "/",
1673+
},
1674+
{
1675+
id: "parent",
1676+
path: "/parent",
1677+
unstable_middleware: [
1678+
async (_, next) => {
1679+
let result = await next();
1680+
expect(isDataWithResponseInit(result)).toBe(true);
1681+
return result;
1682+
},
1683+
async (_, next) => {
1684+
return data("not found", { status: 404 });
1685+
},
1686+
],
1687+
loader() {
1688+
return "PARENT";
1689+
},
1690+
},
1691+
]);
1692+
1693+
let res = (await handler.query(new Request("http://localhost/parent"), {
1694+
unstable_respond: respondWithJson,
1695+
})) as Response;
1696+
expect(res.status).toBe(404);
1697+
await expect(res.text()).resolves.toEqual("not found");
1698+
});
1699+
16341700
describe("ordering", () => {
16351701
it("runs middleware sequentially before and after loaders", async () => {
16361702
let handler = createStaticHandler([
@@ -2512,6 +2578,99 @@ describe("context/middleware", () => {
25122578
expect(res.headers.get("child2")).toEqual("yes");
25132579
});
25142580

2581+
it("propagates the response even if you call next and forget to return it", async () => {
2582+
let handler = createStaticHandler([
2583+
{
2584+
path: "/",
2585+
},
2586+
{
2587+
id: "parent",
2588+
path: "/parent",
2589+
unstable_middleware: [
2590+
async (_, next) => {
2591+
let res = (await next()) as Response;
2592+
res.headers.set("parent", "yes");
2593+
},
2594+
],
2595+
loader() {
2596+
return new Response("PARENT");
2597+
},
2598+
},
2599+
]);
2600+
2601+
let res = (await handler.queryRoute(
2602+
new Request("http://localhost/parent"),
2603+
{
2604+
unstable_respond: (v) => v,
2605+
},
2606+
)) as Response;
2607+
2608+
expect(await res.text()).toBe("PARENT");
2609+
expect(res.headers.get("parent")).toEqual("yes");
2610+
});
2611+
2612+
it("propagates a returned response if next isn't called", async () => {
2613+
let handler = createStaticHandler([
2614+
{
2615+
path: "/",
2616+
},
2617+
{
2618+
id: "parent",
2619+
path: "/parent",
2620+
unstable_middleware: [
2621+
async (_, next) => {
2622+
return new Response("test");
2623+
},
2624+
],
2625+
loader() {
2626+
return "PARENT";
2627+
},
2628+
},
2629+
]);
2630+
2631+
let res = (await handler.queryRoute(
2632+
new Request("http://localhost/parent"),
2633+
{
2634+
unstable_respond: (v) => v,
2635+
},
2636+
)) as Response;
2637+
await expect(res.text()).resolves.toEqual("test");
2638+
});
2639+
2640+
it("propagates a returned data() response if next isn't called", async () => {
2641+
let handler = createStaticHandler([
2642+
{
2643+
path: "/",
2644+
},
2645+
{
2646+
id: "parent",
2647+
path: "/parent",
2648+
unstable_middleware: [
2649+
async (_, next) => {
2650+
let result = await next();
2651+
expect(isDataWithResponseInit(result)).toBe(true);
2652+
return result;
2653+
},
2654+
async (_, next) => {
2655+
return data("not found", { status: 404 });
2656+
},
2657+
],
2658+
loader() {
2659+
return "PARENT";
2660+
},
2661+
},
2662+
]);
2663+
2664+
let res = (await handler.queryRoute(
2665+
new Request("http://localhost/parent"),
2666+
{
2667+
unstable_respond: (v) => v,
2668+
},
2669+
)) as Response;
2670+
expect(res.status).toBe(404);
2671+
await expect(res.text()).resolves.toEqual("not found");
2672+
});
2673+
25152674
describe("ordering", () => {
25162675
it("runs middleware sequentially before and after loaders", async () => {
25172676
let handler = createStaticHandler([

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

Lines changed: 58 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -3544,7 +3544,7 @@ export function createStaticHandler(
35443544
? requestContext
35453545
: new unstable_RouterContextProvider();
35463546

3547-
let respondOrStreamStaticContext = (
3547+
let shortCircuitResult = (
35483548
ctx: StaticHandlerContext,
35493549
): MaybePromise<Response> | StaticHandlerContext => {
35503550
return stream
@@ -3574,7 +3574,7 @@ export function createStaticHandler(
35743574
loaderHeaders: {},
35753575
actionHeaders: {},
35763576
};
3577-
return respondOrStreamStaticContext(staticContext);
3577+
return shortCircuitResult(staticContext);
35783578
} else if (!matches) {
35793579
let error = getInternalRouterError(404, { pathname: location.pathname });
35803580
let { matches: notFoundMatches, route } =
@@ -3592,7 +3592,7 @@ export function createStaticHandler(
35923592
loaderHeaders: {},
35933593
actionHeaders: {},
35943594
};
3595-
return respondOrStreamStaticContext(staticContext);
3595+
return shortCircuitResult(staticContext);
35963596
}
35973597

35983598
if (
@@ -3749,7 +3749,9 @@ export function createStaticHandler(
37493749
? routeId
37503750
: findNearestBoundary(matches, routeId).route.id,
37513751
);
3752-
return respondOrStreamStaticContext(staticContext);
3752+
return stream
3753+
? stream(requestContext, () => Promise.resolve(staticContext))
3754+
: respond!(staticContext);
37533755
} else {
37543756
// We never even got to the handlers, so we've got no data -
37553757
// just create an empty context reflecting the error.
@@ -3779,7 +3781,9 @@ export function createStaticHandler(
37793781
actionHeaders: {},
37803782
loaderHeaders: {},
37813783
};
3782-
return respondOrStreamStaticContext(staticContext);
3784+
return stream
3785+
? stream(requestContext, () => Promise.resolve(staticContext))
3786+
: respond!(staticContext);
37833787
}
37843788
},
37853789
);
@@ -5543,7 +5547,12 @@ export async function runMiddlewarePipeline<T extends boolean>(
55435547
handler: () => T extends true
55445548
? MaybePromise<Response>
55455549
: MaybePromise<Record<string, DataStrategyResult>>,
5546-
errorHandler: (error: unknown, routeId: string) => unknown,
5550+
errorHandler: (
5551+
error: unknown,
5552+
routeId: string,
5553+
) => T extends true
5554+
? MaybePromise<Response>
5555+
: Record<string, DataStrategyResult>,
55475556
): Promise<unknown> {
55485557
let { matches, request, params, context } = args;
55495558
let middlewareState: MutableMiddlewareState = {
@@ -5562,7 +5571,26 @@ export async function runMiddlewarePipeline<T extends boolean>(
55625571
middlewareState,
55635572
handler,
55645573
);
5565-
return propagateResult ? result : middlewareState.handlerResult;
5574+
5575+
if (propagateResult) {
5576+
invariant(
5577+
isResponse(result) || isDataWithResponseInit(result),
5578+
`Expected a Response to be returned from route middleware`,
5579+
);
5580+
// Upgrade returned data() calls to real Responses
5581+
if (isDataWithResponseInit(result)) {
5582+
return new Response(
5583+
typeof result.data === "string"
5584+
? result.data
5585+
: JSON.stringify(result.data),
5586+
{ ...result.init },
5587+
);
5588+
} else {
5589+
return result;
5590+
}
5591+
} else {
5592+
return middlewareState.handlerResult;
5593+
}
55665594
} catch (e) {
55675595
if (!middlewareState.middlewareError) {
55685596
// This shouldn't happen? This would have to come from a bug in our
@@ -5638,18 +5666,33 @@ async function callRouteMiddleware(
56385666
},
56395667
next,
56405668
);
5641-
if (nextCalled) {
5642-
if (result === undefined) {
5669+
5670+
if (propagateResult) {
5671+
// On the server, handle calling next() if needed and returning the proper result
5672+
if (nextCalled) {
56435673
// If they called next() but didn't return the response, we can bubble
5644-
// it for them. This lets folks do things like grab the response and
5645-
// add a header without then re-returning it
5646-
return nextResult;
5647-
} else {
5674+
// it for them. This allows some minor syntactic sugar (or forgetfulness)
5675+
// where you can grab the response to add a header without re-returning it
5676+
return typeof result === "undefined" ? nextResult : result;
5677+
} else if (isResponse(result) || isDataWithResponseInit(result)) {
5678+
// Use short circuit Response/data() values without having called next()
56485679
return result;
5680+
} else {
5681+
// Otherwise call next() for them
5682+
nextResult = await next();
5683+
return nextResult;
56495684
}
56505685
} else {
5651-
result = await next();
5652-
return result;
5686+
// On the client, just call next if they didn't
5687+
if (typeof result !== "undefined") {
5688+
console.warn(
5689+
"client middlewares are not intended to return values, the value will be ignored",
5690+
result,
5691+
);
5692+
}
5693+
if (!nextCalled) {
5694+
await next();
5695+
}
56535696
}
56545697
} catch (error) {
56555698
if (!middlewareState.middlewareError) {

0 commit comments

Comments
 (0)