Skip to content

Properly handle empty body response codes in single fetch requests #12760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 21, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fifty-eagles-love.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"react-router": patch
---

Properly handle status codes that cannot have a body in single fetch responses (204, etc.)
63 changes: 63 additions & 0 deletions integration/single-fetch-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1700,6 +1700,69 @@ test.describe("single-fetch", () => {
]);
});

test("does not try to encode a turbo-stream body into 204 responses", async ({
page,
}) => {
let fixture = await createFixture({
files: {
...files,
"app/routes/_index.tsx": js`
import { data, Form, useActionData, useNavigation } from "react-router";

export async function action({ request }) {
await new Promise(r => setTimeout(r, 500));
return data(null, { status: 204 });
};

export default function Index() {
const navigation = useNavigation();
const actionData = useActionData();
return (
<Form method="post">
{navigation.state === "idle" ? <p data-idle>idle</p> : <p data-active>active</p>}
<button data-submit type="submit">{actionData ?? 'no content!'}</button>
</Form>
);
}
`,
},
});
let appFixture = await createAppFixture(fixture);

let app = new PlaywrightFixture(appFixture, page);

let requests: [string, number, string][] = [];
page.on("request", async (req) => {
if (req.url().includes(".data")) {
let url = new URL(req.url());
requests.push([
req.method(),
(await req.response())!.status(),
url.pathname + url.search,
]);
}
});

// Document requests
let documentRes = await fixture.requestDocument("/?index", {
method: "post",
});
expect(documentRes.status).toBe(204);
expect(await documentRes.text()).toBe("");

// Data requests
await app.goto("/");
(await page.$("[data-submit]"))?.click();
await page.waitForSelector("[data-active]");
await page.waitForSelector("[data-idle]");

expect(await page.innerText("[data-submit]")).toEqual("no content!");
expect(requests).toEqual([
["POST", 204, "/_root.data?index"],
["GET", 200, "/_root.data"],
]);
});

test("does not try to encode a turbo-stream body into 304 responses", async () => {
let fixture = await createFixture({
files: {
Expand Down
21 changes: 20 additions & 1 deletion packages/react-router/lib/dom/ssr/single-fetch.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,10 @@ export function singleFetchUrl(reqUrl: URL | string) {
return url;
}

async function fetchAndDecode(url: URL, init: RequestInit) {
async function fetchAndDecode(
url: URL,
init: RequestInit
): Promise<{ status: number; data: unknown }> {
let res = await fetch(url, init);

// If this 404'd without hitting the running server (most likely in a
Expand All @@ -423,6 +426,22 @@ async function fetchAndDecode(url: URL, init: RequestInit) {
throw new ErrorResponseImpl(404, "Not Found", true);
}

// some status codes are not permitted to have bodies, so we want to just
// treat those as "no data" instead of throwing an exception.
// 304 is not included here because the browser should fill those responses
// with the cached body content.
const NO_BODY_STATUS_CODES = new Set([100, 101, 204, 205]);
if (NO_BODY_STATUS_CODES.has(res.status)) {
if (!init.method || init.method === "GET") {
// SingleFetchResults can just have no routeId keys which will result
// in no data for all routes
return { status: res.status, data: {} };
} else {
// SingleFetchResult is for a singular route and can specify no data
return { status: res.status, data: { data: undefined } };
}
}
Comment on lines +429 to +443
Copy link

@clovis1122 clovis1122 Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: NVM, I misunderstood the spec. This is great.


invariant(res.body, "No response body to decode");

try {
Expand Down
23 changes: 17 additions & 6 deletions packages/react-router/lib/server-runtime/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,17 @@ export type CreateRequestHandlerFunction = (
mode?: string
) => RequestHandler;

// Do not include a response body if the status code is one of these,
// otherwise `undici` will throw an error when constructing the Response:
// https://github.com/nodejs/undici/blob/bd98a6303e45d5e0d44192a93731b1defdb415f3/lib/web/fetch/response.js#L522-L528
//
// Specs:
// https://datatracker.ietf.org/doc/html/rfc9110#name-informational-1xx
// https://datatracker.ietf.org/doc/html/rfc9110#name-204-no-content
// https://datatracker.ietf.org/doc/html/rfc9110#name-205-reset-content
// https://datatracker.ietf.org/doc/html/rfc9110#name-304-not-modified
const NO_BODY_STATUS_CODES = new Set([100, 101, 204, 205, 304]);

function derive(build: ServerBuild, mode?: string) {
let routes = createRoutes(build.routes);
let dataRoutes = createStaticHandlerDataRoutes(build.routes, build.future);
Expand Down Expand Up @@ -297,9 +308,9 @@ async function handleSingleFetchRequest(
let resultHeaders = new Headers(headers);
resultHeaders.set("X-Remix-Response", "yes");

// 304 responses should not have a body
if (status === 304) {
return new Response(null, { status: 304, headers: resultHeaders });
// Skip response body for unsupported status codes
if (NO_BODY_STATUS_CODES.has(status)) {
return new Response(null, { status, headers: resultHeaders });
}

// We use a less-descriptive `text/x-script` here instead of something like
Expand Down Expand Up @@ -347,9 +358,9 @@ async function handleDocumentRequest(

let headers = getDocumentHeaders(build, context);

// 304 responses should not have a body or a content-type
if (context.statusCode === 304) {
return new Response(null, { status: 304, headers });
// Skip response body for unsupported status codes
if (NO_BODY_STATUS_CODES.has(context.statusCode)) {
return new Response(null, { status: context.statusCode, headers });
}

// Sanitize errors outside of development environments
Expand Down
Loading