Skip to content

Commit

Permalink
Properly handle empty body response codes in single fetch requests
Browse files Browse the repository at this point in the history
  • Loading branch information
brophdawg11 committed Jan 16, 2025
1 parent b045242 commit 9b7465a
Show file tree
Hide file tree
Showing 4 changed files with 96 additions and 4 deletions.
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.)
58 changes: 58 additions & 0 deletions integration/single-fetch-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1700,6 +1700,64 @@ 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>
);
}
`,
},
},
ServerMode.Development
);
let appFixture = await createAppFixture(fixture, ServerMode.Development);

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,
]);
}
});

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 } };
}
}

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

try {
Expand Down
16 changes: 13 additions & 3 deletions packages/react-router/lib/server-runtime/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -297,9 +297,19 @@ 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 });
// 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]);
if (NO_BODY_STATUS_CODES.has(status)) {
console.log("returning single fetch response with no body");
return new Response(null, { status, headers: resultHeaders });
}

// We use a less-descriptive `text/x-script` here instead of something like
Expand Down

0 comments on commit 9b7465a

Please sign in to comment.