From bc2924d7de4905ed83883cfce94f831f3cfd6f76 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 16 Jan 2025 13:46:26 -0500 Subject: [PATCH 1/2] Properly handle empty responses in single fetch reqeusts --- .changeset/giant-dolls-wonder.md | 6 +++ integration/single-fetch-test.ts | 58 +++++++++++++++++++++++++ packages/remix-react/single-fetch.tsx | 22 +++++++++- packages/remix-server-runtime/server.ts | 15 +++++-- 4 files changed, 97 insertions(+), 4 deletions(-) create mode 100644 .changeset/giant-dolls-wonder.md diff --git a/.changeset/giant-dolls-wonder.md b/.changeset/giant-dolls-wonder.md new file mode 100644 index 00000000000..c26c5eae49f --- /dev/null +++ b/.changeset/giant-dolls-wonder.md @@ -0,0 +1,6 @@ +--- +"@remix-run/react": patch +"@remix-run/server-runtime": patch +--- + +Properly handle status codes that cannot have a body in single fetch responses (204, etc.) diff --git a/integration/single-fetch-test.ts b/integration/single-fetch-test.ts index 2d4da605ac8..663bf5f9b2f 100644 --- a/integration/single-fetch-test.ts +++ b/integration/single-fetch-test.ts @@ -1931,6 +1931,64 @@ test.describe("single-fetch", () => { ]); }); + test("does not try to encode a turbo-stream body into 204 responses", async ({ + page, + }) => { + let fixture = await createFixture({ + config: { + future: { + v3_singleFetch: true, + }, + }, + files: { + ...files, + "app/routes/_index.tsx": js` + import { data, Form, useActionData, useNavigation } from "@remix-run/react"; + 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 ( +
+ {navigation.state === "idle" ?

idle

:

active

} + +
+ ); + } + `, + }, + }); + 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, + ]); + } + }); + + await app.goto("/", true); + (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({ config: { diff --git a/packages/remix-react/single-fetch.tsx b/packages/remix-react/single-fetch.tsx index 6a47b80c6be..2c5a877db56 100644 --- a/packages/remix-react/single-fetch.tsx +++ b/packages/remix-react/single-fetch.tsx @@ -389,13 +389,33 @@ 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); // Don't do a hard check against the header here. We'll get `text/x-script` // when we have a running server, but if folks want to prerender `.data` files // and serve them from a CDN we should let them come back with whatever // Content-Type their CDN provides and not force them to make sure `.data` // files are served as `text/x-script`. We'll throw if we can't decode anyway. + + // 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. + let 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: null } }; + } + } + invariant(res.body, "No response body to decode"); try { let decoded = await decodeViaTurboStream(res.body, window); diff --git a/packages/remix-server-runtime/server.ts b/packages/remix-server-runtime/server.ts index 2451d701832..22907a1b12e 100644 --- a/packages/remix-server-runtime/server.ts +++ b/packages/remix-server-runtime/server.ts @@ -410,9 +410,18 @@ 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 + let NO_BODY_STATUS_CODES = new Set([100, 101, 204, 205, 304]); + 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 From dc6966a29032c485f95a806e24d5804e6524de49 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 17 Jan 2025 12:35:04 -0500 Subject: [PATCH 2/2] Support 204/205 on document requests --- integration/single-fetch-test.ts | 8 ++++++++ packages/remix-server-runtime/server.ts | 14 +++++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/integration/single-fetch-test.ts b/integration/single-fetch-test.ts index 663bf5f9b2f..a116d8feb32 100644 --- a/integration/single-fetch-test.ts +++ b/integration/single-fetch-test.ts @@ -1977,6 +1977,14 @@ test.describe("single-fetch", () => { } }); + // 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("/", true); (await page.$("[data-submit]"))?.click(); await page.waitForSelector("[data-active]"); diff --git a/packages/remix-server-runtime/server.ts b/packages/remix-server-runtime/server.ts index 22907a1b12e..24031634510 100644 --- a/packages/remix-server-runtime/server.ts +++ b/packages/remix-server-runtime/server.ts @@ -471,9 +471,17 @@ 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 }); + // 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-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 + let NO_BODY_STATUS_CODES = new Set([204, 205, 304]); + if (NO_BODY_STATUS_CODES.has(context.statusCode)) { + return new Response(null, { status: context.statusCode, headers }); } // Sanitize errors outside of development environments