From db4dc98f592c33fa22c2f49c353ac9e49913ccb1 Mon Sep 17 00:00:00 2001 From: Kyle June Date: Wed, 23 Sep 2026 05:19:59 -0400 Subject: [PATCH] fix: keep headers of thrown and data() responses On a data request, a loader or action that threw a non-redirect Response, or threw or returned data(), made server.request reject: React Router's queryRoute throws a Response for those, and Hono only routes Error instances to onError. A data-request dataStrategy now turns a thrown Response or data() into an HttpError (the data-error envelope, with its status and headers) and a returned data() into a 200 data envelope with its headers. Both envelopes go through commitResponse. On a document request, the error document now carries the headers of the HttpError that sets its status, not only one thrown by middleware. Co-Authored-By: Claude Opus 5.5 --- docs/error-handling.md | 22 ++++ docs/routing.md | 5 + src/_server.tsx | 219 ++++++++++++++++++++++++---------- src/server.test.tsx | 262 +++++++++++++++++++++++++++++++++++++++++ src/server.tsx | 5 +- 5 files changed, 450 insertions(+), 63 deletions(-) diff --git a/docs/error-handling.md b/docs/error-handling.md index 6c717e3..1c67204 100644 --- a/docs/error-handling.md +++ b/docs/error-handling.md @@ -95,6 +95,28 @@ export function ErrorBoundary({ error }: ErrorBoundaryProps) { } ``` +### Error Headers + +Pass `headers` to send response headers with an error, such as a cookie that +signs the user out or a cache policy: + +```typescript +import { HttpError } from "@udibo/juniper"; + +const headers = new Headers({ "Cache-Control": "no-store" }); +headers.append("Set-Cookie", "session=; Max-Age=0; Path=/"); +throw new HttpError(401, { message: "Signed out", headers }); +``` + +When a loader or action throws the error, the error document or the data +request's error response carries those headers. Every `Set-Cookie` value is +kept. + +A loader or action can also throw a `Response` other than a redirect, or throw +`data()` from React Router. On a data request, Juniper sends it as an +`HttpError` with that status and those headers. A status outside 400–599 becomes +`500`. + ## Error Boundaries Error boundaries catch errors thrown during rendering, in loaders, actions, or diff --git a/docs/routing.md b/docs/routing.md index c1fa4f4..1c5987c 100644 --- a/docs/routing.md +++ b/docs/routing.md @@ -521,6 +521,11 @@ A few other cases: throws the redirect or returns it. - A `Response` other than a redirect that a loader or action returns keeps its own headers. Juniper adds no default policy to it. +- `data()` from React Router that a loader or action returns arrives on a data + request as data with a `200` status, because the client reads any other status + as an error. Its headers are kept, and a `Cache-Control` header among them is + used instead of the middleware policy or the default. Its status applies to + document requests. ### Client Loaders diff --git a/src/_server.tsx b/src/_server.tsx index e05d18b..9f0a8af 100644 --- a/src/_server.tsx +++ b/src/_server.tsx @@ -18,6 +18,8 @@ import { } from "react-router"; import type { DataRouteObject, + DataStrategyFunctionArgs, + DataStrategyResult, RouterContextProvider, StaticHandlerContext, } from "react-router"; @@ -306,6 +308,48 @@ function getPublicEnv(allPublicEnvKeys: string[]): Record { return publicEnv; } +interface DataWithResponseInit { + type: "DataWithResponseInit"; + data: unknown; + init: ResponseInit | null; +} + +function isDataWithResponseInit( + value: unknown, +): value is DataWithResponseInit { + return typeof value === "object" && value !== null && + (value as { type?: unknown }).type === "DataWithResponseInit"; +} + +function errorStatus(status: number | undefined): number { + return status !== undefined && status >= 400 && status < 600 ? status : 500; +} + +const BODY_HEADERS = new Set([ + "content-type", + "content-length", + "content-encoding", + "transfer-encoding", + "x-juniper", +]); + +async function responseToHttpError(response: Response): Promise { + const headers = new Headers(response.headers); + const contentType = response.headers.get("content-type"); + if (contentType?.includes("application/problem+json")) { + const error = HttpError.from({ + ...await response.json(), + status: errorStatus(response.status), + }); + error.headers = headers; + return error; + } + return new HttpError(errorStatus(response.status), { + message: await response.text(), + headers, + }); +} + async function convertToHttpError(cause: unknown): Promise { if ( cause !== null && @@ -314,24 +358,15 @@ async function convertToHttpError(cause: unknown): Promise { "getResponse" in cause && typeof cause.getResponse === "function" ) { - const response = cause.getResponse() as Response; - const status = response.status; - const headers = new Headers(response.headers); - - let message: string | undefined; - const contentType = response.headers.get("content-type"); - if (contentType?.includes("application/problem+json")) { - return HttpError.from({ - status, - ...await response.json(), - }); - } else { - message = await response.text(); - } - - const error = new HttpError(status, { message, headers }); - error.headers = headers; - return error; + return await responseToHttpError(cause.getResponse() as Response); + } + if (cause instanceof Response) return await responseToHttpError(cause); + if (isDataWithResponseInit(cause)) { + const { data, init } = cause; + return new HttpError(errorStatus(init?.status), { + message: typeof data === "string" ? data : init?.statusText, + headers: new Headers(init?.headers), + }); } return HttpError.from(cause); } @@ -513,12 +548,17 @@ async function renderDocument( const actionHeaders = context.actionHeaders[deepestMatch.route.id]; const loaderHeaders = context.loaderHeaders[deepestMatch.route.id]; - const statusCode = (presetError?.status ?? - Object.values(context.errors ?? {}) - .find((value: unknown) => isHttpErrorLike(value)) - ?.status ?? + const reportedError = presetError ?? + Object.values(context.errors ?? {}).find((value: unknown) => + isHttpErrorLike(value) + ); + const statusCode = (reportedError?.status ?? context.statusCode ?? 200) as StatusCode; + const errorHeaders = reportedError && "headers" in reportedError && + reportedError.headers instanceof Headers + ? reportedError.headers + : undefined; c.status(statusCode); @@ -539,16 +579,11 @@ async function renderDocument( c.header("Set-Cookie", cookie, { append: true }); } - if (presetError?.headers) { - for (const [key, value] of presetError.headers.entries()) { - if ( - key.toLowerCase() !== "content-type" && - key.toLowerCase() !== "set-cookie" - ) { - c.header(key, value); - } + if (errorHeaders) { + for (const [key, value] of errorHeaders) { + if (!BODY_HEADERS.has(key) && key !== "set-cookie") c.header(key, value); } - for (const cookie of presetError.headers.getSetCookie()) { + for (const cookie of errorHeaders.getSetCookie()) { c.header("Set-Cookie", cookie, { append: true }); } } @@ -927,14 +962,91 @@ function newDataResponse( const defaultPolicy = headers.get("Cache-Control") ?? ""; headers.delete("Cache-Control"); const response = c.newResponse(body, { ...init, headers }); - const policy = ownPolicy ?? dataCachePolicy( - response.headers.get("Cache-Control"), + const policy = dataCachePolicy( + ownPolicy ?? response.headers.get("Cache-Control"), defaultPolicy, ); response.headers.set("Cache-Control", policy); return commitResponse(c, response); } +function newEnvelopeResponse( + c: Context, + envelope: Response, + status: StatusCode, + ownHeaders?: Headers, +): Response { + const headers = new Headers(); + for (const [key, value] of ownHeaders ?? []) { + if (!BODY_HEADERS.has(key) && key !== "set-cookie") { + headers.set(key, value); + } + } + for (const cookie of ownHeaders?.getSetCookie() ?? []) { + headers.append("Set-Cookie", cookie); + } + for (const [key, value] of envelope.headers) headers.set(key, value); + return newDataResponse( + c, + envelope.body, + { status, headers }, + ownHeaders?.get("Cache-Control") ?? null, + ); +} + +class DataWithHeaders { + constructor(readonly data: unknown, readonly headers: Headers) {} +} + +const ROUTER_REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]); + +function isRedirect(result: unknown): boolean { + const init = result instanceof Response + ? result + : isDataWithResponseInit(result) + ? result.init + : undefined; + return ROUTER_REDIRECT_STATUSES.has(init?.status ?? 200) && + new Headers(init?.headers).has("Location"); +} + +async function toDataRequestResult( + settled: DataStrategyResult, +): Promise { + const { type, result } = settled; + if (isRedirect(result)) return settled; + if ( + type === "error" && + (result instanceof Response || isDataWithResponseInit(result)) + ) { + return { type, result: await convertToHttpError(result) }; + } + if (type === "data" && isDataWithResponseInit(result)) { + return { + type, + result: new DataWithHeaders( + result.data, + new Headers(result.init?.headers), + ), + }; + } + return settled; +} + +async function dataRequestStrategy( + { matches }: DataStrategyFunctionArgs, +): Promise> { + const results: Record = {}; + await Promise.all( + matches.filter((match) => match.shouldLoad).map(async (match) => { + results[match.route.id] = await toDataRequestResult( + await match.resolve(), + ); + }), + ); + return results; +} + /** * Builds the Hono handlers for the client routes — server-rendered documents * and data requests — plus the error handler `createServer` installs alongside @@ -1001,6 +1113,7 @@ export function createHandlers< const dataOrResponse = await queryRoute(c.req.raw, { requestContext, routeId, + dataStrategy: dataRequestStrategy, }); if (dataOrResponse instanceof Response) { @@ -1016,14 +1129,15 @@ export function createHandlers< ); } - const response = createLoaderDataResponse( - dataOrResponse, - c.req.raw.signal, + const { data, headers } = dataOrResponse instanceof DataWithHeaders + ? dataOrResponse + : { data: dataOrResponse, headers: undefined }; + return newEnvelopeResponse( + c, + createLoaderDataResponse(data, c.req.raw.signal), + 200, + headers, ); - return newDataResponse(c, response.body, { - status: response.status as StatusCode, - headers: response.headers, - }); }); }, ); @@ -1039,28 +1153,11 @@ export function createHandlers< console.error(error); if (c.req.header("X-Juniper-Route-Id")) { - const serialized = serializeError(error); - const response = createLoaderDataResponse(serialized, c.req.raw.signal); - const headers = new Headers(response.headers); - if (error.headers) { - for (const [key, value] of error.headers.entries()) { - if ( - !["content-type", "content-length", "content-encoding", "x-juniper"] - .includes(key.toLowerCase()) && - key.toLowerCase() !== "set-cookie" - ) { - headers.set(key, value); - } - } - for (const cookie of error.headers.getSetCookie()) { - headers.append("Set-Cookie", cookie); - } - } - return newDataResponse( + return newEnvelopeResponse( c, - response.body, - { status: error.status as StatusCode, headers }, - error.headers?.get("Cache-Control") ?? null, + createLoaderDataResponse(serializeError(error), c.req.raw.signal), + error.status as StatusCode, + error.headers, ); } diff --git a/src/server.test.tsx b/src/server.test.tsx index dc5a183..30e4474 100644 --- a/src/server.test.tsx +++ b/src/server.test.tsx @@ -2,6 +2,7 @@ import { assertEquals, assertExists, assertFalse, + assertInstanceOf, assertNotEquals, assertStringIncludes, } from "@std/assert"; @@ -9,6 +10,7 @@ import { describe, it } from "@std/testing/bdd"; import { stub } from "@std/testing/mock"; import * as path from "@std/path"; import { + data, Outlet, redirect, redirectDocument, @@ -34,6 +36,7 @@ import { mergeServerRoutes, } from "./_server.tsx"; import { + deserializeError, deserializeHydrationData, deserializeLoaderData, } from "./_serialization.ts"; @@ -1467,6 +1470,265 @@ describe("the headers a loader or action response sets itself", () => { assertEquals(response.status, 401); assertOwnHeadersKept(response); }); + + for (const method of ["GET", "POST"]) { + const dataRequest = { + method, + headers: { "X-Juniper-Route-Id": "/" }, + }; + + it(`sends a Response a ${method} handler throws on a data request as a data error with its status and headers`, async () => { + using _log = stub(console, "error"); + const server = serverWithRoute(readsResponseFirst, () => { + throw new Response("Gone for good", { + status: 410, + headers: [...ownHeaders, ["Cache-Control", "no-store"]], + }); + }); + const response = await server.request( + "http://localhost/", + dataRequest, + ); + assertEquals(response.status, 410); + assertEquals(response.headers.get("X-Juniper"), "data"); + const error = deserializeError( + deserializeLoaderData(await response.text()), + ); + assertInstanceOf(error, HttpError); + assertEquals(error.status, 410); + assertEquals(error.message, "Gone for good"); + assertEquals(response.headers.get("Cache-Control"), "no-store"); + assertOwnHeadersKept(response); + }); + + it(`sends data() a ${method} handler throws on a data request as a data error with its status and headers`, async () => { + using _log = stub(console, "error"); + const server = serverWithRoute(readsResponseFirst, () => { + throw data("Gone for good", { + status: 410, + headers: [...ownHeaders, ["Cache-Control", "no-store"]], + }); + }); + const response = await server.request( + "http://localhost/", + dataRequest, + ); + assertEquals(response.status, 410); + assertEquals(response.headers.get("X-Juniper"), "data"); + const error = deserializeError( + deserializeLoaderData(await response.text()), + ); + assertInstanceOf(error, HttpError); + assertEquals(error.status, 410); + assertEquals(error.message, "Gone for good"); + assertEquals(response.headers.get("Cache-Control"), "no-store"); + assertOwnHeadersKept(response); + }); + + for (const status of [201, 204, 404]) { + it(`sends data() with a ${status} status a ${method} handler returns on a data request as data with its headers`, async () => { + const server = serverWithRoute( + readsResponseFirst, + () => + data({ saved: new Date(0) }, { + status, + headers: [...ownHeaders, ["Cache-Control", "no-store"]], + }), + ); + const response = await server.request( + "http://localhost/", + dataRequest, + ); + assertEquals(response.status, 200); + assertEquals(response.headers.get("X-Juniper"), "data"); + assertEquals( + deserializeLoaderData(await response.text()), + { saved: new Date(0) }, + ); + assertEquals(response.headers.get("Cache-Control"), "no-store"); + assertOwnHeadersKept(response); + }); + } + + it(`applies the app's cache policy to data() a ${method} handler returns without one`, async () => { + const server = serverWithRoute( + readsResponseFirst, + () => data({ saved: true }, { headers: ownHeaders }), + ); + const response = await server.request( + "http://localhost/", + dataRequest, + ); + assertEquals(response.status, 200); + assertEquals(deserializeLoaderData(await response.text()), { + saved: true, + }); + assertEquals( + response.headers.get("Cache-Control"), + "public, max-age=60", + ); + assertOwnHeadersKept(response); + }); + + it(`sends a problem details Response a ${method} handler throws on a data request with its own status and headers`, async () => { + using _log = stub(console, "error"); + const server = serverWithRoute(readsResponseFirst, () => { + throw new Response( + JSON.stringify({ + status: 400, + title: "Unprocessable", + detail: "Bad input", + }), + { + status: 422, + headers: [ + ...ownHeaders, + ["Content-Type", "application/problem+json"], + ], + }, + ); + }); + const response = await server.request( + "http://localhost/", + dataRequest, + ); + assertEquals(response.status, 422); + const error = deserializeError( + deserializeLoaderData(await response.text()), + ); + assertInstanceOf(error, HttpError); + assertEquals(error.status, 422); + assertEquals(error.message, "Bad input"); + assertOwnHeadersKept(response); + }); + + it(`adds no-transform to the cache policy of deferred data() a ${method} handler returns`, async () => { + const server = serverWithRoute( + readsResponseFirst, + () => + data({ later: Promise.resolve("ready") }, { + headers: { "Cache-Control": "private, max-age=5" }, + }), + ); + const response = await server.request( + "http://localhost/", + dataRequest, + ); + await response.arrayBuffer(); + assertEquals( + response.headers.get("Content-Type"), + "application/x-ndjson", + ); + assertEquals( + response.headers.get("Cache-Control"), + "private, max-age=5, no-transform", + ); + }); + + it(`sends data() with a 3xx status React Router does not redirect for, that a ${method} handler returns, as data`, async () => { + const server = serverWithRoute( + readsResponseFirst, + () => + data({ saved: true }, { + status: 305, + headers: [...ownHeaders, ["Location", "/proxy"]], + }), + ); + const response = await server.request( + "http://localhost/", + dataRequest, + ); + assertEquals(response.status, 200); + assertEquals(response.headers.get("X-Juniper"), "data"); + assertEquals(deserializeLoaderData(await response.text()), { + saved: true, + }); + assertOwnHeadersKept(response); + }); + + it(`sends data() with a redirect status a ${method} handler returns on a data request as a redirect`, async () => { + const server = serverWithRoute( + readsResponseFirst, + () => + data(null, { + status: 302, + headers: [...ownHeaders, ["Location", "/sign-in"]], + }), + ); + const response = await server.request( + "http://localhost/", + dataRequest, + ); + assertEquals(response.status, 200); + assertEquals(response.headers.get("X-Juniper"), "redirect"); + assertEquals(await response.json(), { location: "/sign-in" }); + assertOwnHeadersKept(response); + }); + + it(`keeps every header of an HttpError a ${method} handler throws on a document request, after the app's`, async () => { + using _log = stub(console, "error"); + const server = serverWithRoute(readsResponseFirst, () => { + throw new HttpError(401, { + message: "Signed out", + headers: new Headers([ + ...ownHeaders, + ["Cache-Control", "no-store"], + ]), + }); + }); + const response = await server.request("http://localhost/", { + method, + }); + assertStringIncludes(await response.text(), " { + using _log = stub(console, "error"); + const server = serverWithRoute(readsResponseFirst, () => { + throw new HttpError(401, { + message: "Signed out", + headers: new Headers([ + ...ownHeaders, + ["Content-Length", "12"], + ["Content-Encoding", "gzip"], + ]), + }); + }); + const response = await server.request("http://localhost/", { + method, + }); + await response.arrayBuffer(); + assertEquals(response.status, 401); + assertEquals(response.headers.get("Content-Length"), null); + assertEquals(response.headers.get("Content-Encoding"), null); + assertOwnHeadersKept(response); + }); + } + + it("sends a thrown Response without an error status as a server error", async () => { + using _log = stub(console, "error"); + const server = serverWithRoute(readsResponseFirst, () => { + throw new Response("Not an error", { headers: ownHeaders }); + }); + const response = await server.request("http://localhost/", { + headers: { "X-Juniper-Route-Id": "/" }, + }); + assertEquals(response.status, 500); + assertEquals(response.headers.get("X-Juniper"), "data"); + const error = deserializeError( + deserializeLoaderData(await response.text()), + ); + assertInstanceOf(error, HttpError); + assertEquals(error.status, 500); + assertOwnHeadersKept(response); + }); }); } }); diff --git a/src/server.tsx b/src/server.tsx index 59f63c4..56edf46 100644 --- a/src/server.tsx +++ b/src/server.tsx @@ -55,8 +55,9 @@ function varyByRoute(headers: Headers): void { * `X-Juniper-Route-Id` while retaining application cache variation. Route data * responses and redirects sent to data requests default to `Cache-Control: * private, no-cache`, plus `no-transform` when deferred; a policy route - * middleware sets before `next()` replaces it, and a policy on a redirect a - * loader or action returns or throws replaces both. + * middleware sets before `next()` replaces it, and a policy on a redirect or + * error a loader or action returns or throws, or on the `data()` it returns, + * replaces both. * * @param moduleUrl - File URL of the application entrypoint; its directory owns `public/`. * @param client - Matching client route definitions from the same build.