Skip to content

Commit

Permalink
Properly handle empty responses in single fetch reqeusts
Browse files Browse the repository at this point in the history
  • Loading branch information
brophdawg11 committed Jan 16, 2025
1 parent 0e10e98 commit bc2924d
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 4 deletions.
6 changes: 6 additions & 0 deletions .changeset/giant-dolls-wonder.md
Original file line number Diff line number Diff line change
@@ -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.)
58 changes: 58 additions & 0 deletions integration/single-fetch-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<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,
]);
}
});

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: {
Expand Down
22 changes: 21 additions & 1 deletion packages/remix-react/single-fetch.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
15 changes: 12 additions & 3 deletions packages/remix-server-runtime/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit bc2924d

Please sign in to comment.