Properly handle empty responses in single fetch requests (#10410)

.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.)

.gitignore

@@ -14,7 +14,7 @@ tsconfig.tsbuildinfo
 /fixtures/my-remix-app
 /fixtures/deno-app
 /integration/playwright-report
-/test-results
+/integration/test-results
 /uploads
 
 .eslintcache

integration/package.json

@@ -10,7 +10,7 @@
   "dependencies": {
     "@cloudflare/kv-asset-handler": "^0.3.0",
     "@cloudflare/workers-types": "^4.20230518.0",
-    "@playwright/test": "^1.33.0",
+    "@playwright/test": "^1.49.1",
     "@remix-run/dev": "workspace:*",
     "@remix-run/express": "workspace:*",
     "@remix-run/node": "workspace:*",

integration/single-fetch-test.ts

@@ -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: {

package.json

@@ -53,7 +53,7 @@
     "@octokit/graphql": "^4.8.0",
     "@octokit/plugin-paginate-rest": "^2.17.0",
     "@octokit/rest": "^18.12.0",
-    "@playwright/test": "^1.33.0",
+    "@playwright/test": "^1.49.1",
     "@remix-run/architect": "workspace:*",
     "@remix-run/changelog-github": "^0.0.5",
     "@remix-run/cloudflare": "workspace:*",

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

packages/remix-server-runtime/server.ts

@@ -54,6 +54,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);
@@ -410,9 +421,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
@@ -462,9 +473,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