Skip to content
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

Properly handle empty responses in single fetch requests #10410

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open
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
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.)
66 changes: 66 additions & 0 deletions integration/single-fetch-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1931,6 +1931,72 @@ 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,
]);
}
});

// 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]");
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
29 changes: 23 additions & 6 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 Expand Up @@ -462,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
Expand Down
Loading