From 7fc2d141f5a51b3ae60fa8de5d2c6dda9dc5d21a Mon Sep 17 00:00:00 2001 From: Jacob Ebey Date: Thu, 1 Jun 2023 13:10:35 -0700 Subject: [PATCH 01/11] fix(remix-node): fix `NodeRequest`'s `clone` function (#6512) Co-authored-by: Tomer Yechiel --- .changeset/req-clone-instanceof.md | 6 ++++++ contributors.yml | 1 + packages/remix-node/__tests__/fetch-test.ts | 15 ++++++++++++--- packages/remix-node/fetch.ts | 2 +- 4 files changed, 20 insertions(+), 4 deletions(-) create mode 100644 .changeset/req-clone-instanceof.md diff --git a/.changeset/req-clone-instanceof.md b/.changeset/req-clone-instanceof.md new file mode 100644 index 00000000000..a83a3f1e233 --- /dev/null +++ b/.changeset/req-clone-instanceof.md @@ -0,0 +1,6 @@ +--- +"remix": patch +"@remix-run/node": patch +--- + +Fix `request.clone() instanceof Request` returning false. diff --git a/contributors.yml b/contributors.yml index efe470f9df8..dcac6473ed5 100644 --- a/contributors.yml +++ b/contributors.yml @@ -491,6 +491,7 @@ - tombiju - tombyrer - TomerAberbach +- tomer-yechiel - toyozaki - turkerdev - tvanantwerp diff --git a/packages/remix-node/__tests__/fetch-test.ts b/packages/remix-node/__tests__/fetch-test.ts index 9468160d65a..4fd79135ee5 100644 --- a/packages/remix-node/__tests__/fetch-test.ts +++ b/packages/remix-node/__tests__/fetch-test.ts @@ -1,4 +1,5 @@ import { PassThrough } from "stream"; +import { ReadableStream } from "@remix-run/web-stream"; import { Request } from "../fetch"; @@ -70,9 +71,15 @@ let test = { describe("Request", () => { it("clones", async () => { - let body = new PassThrough(); - test.source.forEach((chunk) => body.write(chunk)); - body.end(); + let encoder = new TextEncoder(); + let body = new ReadableStream({ + start(controller) { + test.source.forEach((chunk) => { + controller.enqueue(encoder.encode(chunk)); + }); + controller.close(); + }, + }); let req = new Request("http://test.com", { method: "post", @@ -103,6 +110,8 @@ describe("Request", () => { file = clonedFormData.get("upload_file_1") as File; expect(file.name).toBe("1k_b.dat"); expect(file.size).toBe(1023); + + expect(cloned instanceof Request).toBeTruthy(); }); }); diff --git a/packages/remix-node/fetch.ts b/packages/remix-node/fetch.ts index ce42c4ed8d3..f586469dd35 100644 --- a/packages/remix-node/fetch.ts +++ b/packages/remix-node/fetch.ts @@ -41,7 +41,7 @@ class NodeRequest extends WebRequest { } public clone(): NodeRequest { - return super.clone() as NodeRequest; + return new NodeRequest(this); } } From 90cbe9a16a77b0cb03b36916e160038ac195528f Mon Sep 17 00:00:00 2001 From: Jacob Ebey Date: Thu, 1 Jun 2023 13:13:22 -0700 Subject: [PATCH 02/11] feat: retry failed revalidation caused by HDR (#6287) --- .changeset/hdr-revalidation-retry.md | 5 +++++ packages/remix-react/browser.tsx | 2 ++ packages/remix-react/data.ts | 24 +++++++++++++++++++++--- 3 files changed, 28 insertions(+), 3 deletions(-) create mode 100644 .changeset/hdr-revalidation-retry.md diff --git a/.changeset/hdr-revalidation-retry.md b/.changeset/hdr-revalidation-retry.md new file mode 100644 index 00000000000..949fb91bcf1 --- /dev/null +++ b/.changeset/hdr-revalidation-retry.md @@ -0,0 +1,5 @@ +--- +"@remix-run/react": patch +--- + +retry HDR revalidations in development mode to aid in 3rd party server race conditions diff --git a/packages/remix-react/browser.tsx b/packages/remix-react/browser.tsx index 821731a3dad..f56510d6e2e 100644 --- a/packages/remix-react/browser.tsx +++ b/packages/remix-react/browser.tsx @@ -30,6 +30,7 @@ declare global { }; var __remixRouteModules: RouteModules; var __remixManifest: EntryContext["manifest"]; + var __remixRevalidation: number | undefined; var $RefreshRuntime$: { performReactRefresh: () => void; }; @@ -139,6 +140,7 @@ if (import.meta && import.meta.hot) { }, 1); } }); + window.__remixRevalidation = (window.__remixRevalidation || 0) + 1; router.revalidate(); } ); diff --git a/packages/remix-react/data.ts b/packages/remix-react/data.ts index a065b65c407..675b8413768 100644 --- a/packages/remix-react/data.ts +++ b/packages/remix-react/data.ts @@ -14,7 +14,7 @@ export function isCatchResponse(response: Response): boolean { return response.headers.get("X-Remix-Catch") != null; } -export function isErrorResponse(response: Response): boolean { +export function isErrorResponse(response: any): response is Response { return response.headers.get("X-Remix-Error") != null; } @@ -28,7 +28,8 @@ export function isDeferredResponse(response: Response): boolean { export async function fetchData( request: Request, - routeId: string + routeId: string, + retry = 0 ): Promise { let url = new URL(request.url); url.searchParams.set("_data", routeId); @@ -47,7 +48,24 @@ export async function fetchData( : await request.formData(); } - let response = await fetch(url.href, init); + if (retry > 0) { + // Retry up to 3 times waiting 50, 250, 1250 ms + // between retries for a total of 1550 ms before giving up. + await new Promise((resolve) => setTimeout(resolve, 5 ** retry * 10)); + } + + let revalidation = window.__remixRevalidation; + let response = await fetch(url.href, init).catch((error) => { + if ( + typeof revalidation === "number" && + revalidation === window.__remixRevalidation && + error?.name === "TypeError" && + retry < 3 + ) { + return fetchData(request, routeId, retry + 1); + } + throw error; + }); if (isErrorResponse(response)) { let data = await response.json(); From c6d8c37a8439e7d9edeac6e948595f29f35f7cc9 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 1 Jun 2023 18:12:19 -0400 Subject: [PATCH 03/11] Fix server error logging and add onUnhandledError support (#6495) --- .changeset/log-server-errors.md | 5 + .changeset/on-unhandled-error.md | 22 + docs/file-conventions/entry.server.md | 47 +- integration/error-sanitization-test.ts | 270 +++++++- integration/headers-test.ts | 590 +++++++++--------- packages/remix-cloudflare/index.ts | 1 + packages/remix-deno/index.ts | 1 + .../utils/export.ts | 1 + .../cloudflare/entry.server.react-stream.tsx | 1 + .../deno/entry.server.react-stream.tsx | 1 + .../node/entry.server.react-stream.tsx | 18 +- .../rules/packageExports.js | 1 + packages/remix-node/index.ts | 1 + packages/remix-react/errorBoundaries.tsx | 27 +- .../__tests__/server-test.ts | 271 ++++---- packages/remix-server-runtime/build.ts | 5 + packages/remix-server-runtime/index.ts | 1 + packages/remix-server-runtime/reexport.ts | 1 + packages/remix-server-runtime/server.ts | 65 +- rollup.utils.js | 1 + templates/arc/app/entry.server.tsx | 18 +- .../cloudflare-pages/app/entry.server.tsx | 1 + .../cloudflare-workers/app/entry.server.tsx | 1 + templates/deno/app/entry.server.tsx | 1 + templates/express/app/entry.server.tsx | 18 +- templates/fly/app/entry.server.tsx | 18 +- templates/netlify/app/entry.server.tsx | 18 +- templates/remix/app/entry.server.tsx | 18 +- templates/vercel/app/entry.server.tsx | 18 +- 29 files changed, 954 insertions(+), 487 deletions(-) create mode 100644 .changeset/log-server-errors.md create mode 100644 .changeset/on-unhandled-error.md diff --git a/.changeset/log-server-errors.md b/.changeset/log-server-errors.md new file mode 100644 index 00000000000..5d646cf5f61 --- /dev/null +++ b/.changeset/log-server-errors.md @@ -0,0 +1,5 @@ +--- +"@remix-run/server-runtime": patch +--- + +Ensure un-sanitized server errors are logged on the server during document requests diff --git a/.changeset/on-unhandled-error.md b/.changeset/on-unhandled-error.md new file mode 100644 index 00000000000..ebd7f740166 --- /dev/null +++ b/.changeset/on-unhandled-error.md @@ -0,0 +1,22 @@ +--- +"@remix-run/server-runtime": minor +--- + +Add optional `onUnhandledError` export for custom server-side error processing. This is a new optional export from your `entry.server.tsx` that will be called with any encountered error on the Remix server (loader, action, or render error): + +```ts +// entry.server.tsx +export function onUnhandledError( + error: unknown, + { request, params, context }: DataFunctionArgs +): void { + if (error instanceof Error) { + sendErrorToBugReportingService(error); + console.error(formatError(error)); + } else { + let unknownError = new Error("Unknown Server Error"); + sendErrorToBugReportingService(unknownError); + console.error(unknownError); + } +} +``` diff --git a/docs/file-conventions/entry.server.md b/docs/file-conventions/entry.server.md index cc25b6729f5..4019095551d 100644 --- a/docs/file-conventions/entry.server.md +++ b/docs/file-conventions/entry.server.md @@ -5,10 +5,53 @@ toc: false # entry.server -By default, Remix will handle generating the HTTP Response for you. If you want to customize this behavior, you can run `npx remix reveal` to generate a `app/entry.server.tsx` (or `.jsx`) that will take precedence. The `default` export of this module is a function that lets you create the response, including HTTP status, headers, and HTML, giving you full control over the way the markup is generated and sent to the client. +By default, Remix will handle generating the HTTP Response for you. If you want to customize this behavior, you can run `npx remix reveal` to generate an `app/entry.server.tsx` (or `.jsx`) that will take precedence. The `default` export of this module is a function that lets you create the response, including HTTP status, headers, and HTML, giving you full control over the way the markup is generated and sent to the client. This module should render the markup for the current page using a `` element with the `context` and `url` for the current request. This markup will (optionally) be re-hydrated once JavaScript loads in the browser using the [browser entry module][browser-entry-module]. -You can also export an optional `handleDataRequest` function that will allow you to modify the response of a data request. These are the requests that do not render HTML, but rather return the loader and action data to the browser once client-side hydration has occurred. +## `handleDataRequest` + +You can export an optional `handleDataRequest` function that will allow you to modify the response of a data request. These are the requests that do not render HTML, but rather return the loader and action data to the browser once client-side hydration has occurred. + +```tsx +export function handleDataRequest( + response: Response, + { request, params, context }: DataFunctionArgs +) { + response.headers.set("X-Custom-Header", "value"); + return response; +} +``` + +## `onUnhandledError` + +By default, Remix will log encountered server-side errors to the console. If you'd like more control over the logging, or would like to also report these errors to an external service, then you can export an optional `onUnhandledError` function which will give you control (and will disable the built-in error logging). + +```tsx +export function onUnhandledError( + error: unknown, + { request, params, context }: DataFunctionArgs +) { + sendErrorToErrorReportingService(error); + console.error(formatErrorForJsonLogging(error)); +} +``` + +### Streaming Rendering Errors + +When you are streaming your HTML responses via [`renderToPipeableStream`][rendertopipeablestream] or [`renderToReadableStream`][rendertoreadablestream], your own `onUnhandledError` implementation will only handle errors encountered uring the initial shell render. If you encounter a rendering error during subsequent streamed rendering you will need handle these errors manually since the Remix server has already sent the Response by that point. + +- For `renderToPipeableStream`, you can handle these errors in the `onError` callback function. You will need to toggle a boolean when the in `onShellReady` so you know if the error was a shell rendering error (and can be ignored) or an async rendering error (and must be handled). + - For an exmaple, please see the default [`entry.server.tsx`][node-streaming-entry-server] for Node. +- For `renderToReadableStream`, you can handle these errors in the `onError` callback function + - For an example, please see the default [`entry.server.tsx`][cloudflare-streaming-entry-server] for Cloudflare + +### Thrown Responses + +Note that this does not handle thrown `Response` instances from your `loader`/`action` functions. The intention of this handler is to find bugs in your code which result in unexpected thrown errors. If you are detecting a scenario and throwing a 401/404/etc. `Response` in your `loader`/`action` then it's an expected flow that is handled by your code. If you also wish to log, or send those to an external service, that should be done at the time you throw the response. [browser-entry-module]: ./entry.client +[rendertopipeablestream]: https://react.dev/reference/react-dom/server/renderToPipeableStream +[rendertoreadablestream]: https://react.dev/reference/react-dom/server/renderToReadableStream +[node-streaming-entry-server]: https://github.com/remix-run/remix/blob/main/packages/remix-dev/config/defaults/node/entry.server.react-stream.tsx +[cloudflare-streaming-entry-server]: https://github.com/remix-run/remix/blob/main/packages/remix-dev/config/defaults/cloudflare/entry.server.react-stream.tsx diff --git a/integration/error-sanitization-test.ts b/integration/error-sanitization-test.ts index 4ed62a49faa..ad918301485 100644 --- a/integration/error-sanitization-test.ts +++ b/integration/error-sanitization-test.ts @@ -27,7 +27,7 @@ const routeFiles = { `, "app/routes/index.jsx": js` - import { useLoaderData, useLocation } from "@remix-run/react"; + import { useLoaderData, useLocation, useRouteError } from "@remix-run/react"; export function loader({ request }) { if (new URL(request.url).searchParams.has('loader')) { @@ -52,7 +52,8 @@ const routeFiles = { ); } - export function ErrorBoundary({ error }) { + export function ErrorBoundary() { + let error = useRouteError(); return ( <>

Index Error

@@ -66,7 +67,7 @@ const routeFiles = { "app/routes/defer.jsx": js` import * as React from 'react'; import { defer } from "@remix-run/server-runtime"; - import { Await, useLoaderData, useRouteError } from "@remix-run/react"; + import { Await, useAsyncError, useLoaderData, useRouteError } from "@remix-run/react"; export function loader({ request }) { if (new URL(request.url).searchParams.has('loader')) { @@ -95,19 +96,20 @@ const routeFiles = { } function AwaitError() { - let error = useRouteError(); + let error = useAsyncError(); return ( <>

Defer Error

-

{error}

+

{error.message}

); } - export function ErrorBoundary({ error }) { + export function ErrorBoundary() { + let error = useRouteError(); return ( <> -

Index Error

+

Defer Error

{"MESSAGE:" + error.message}

{error.stack ?

{"STACK:" + error.stack}

: null} @@ -128,10 +130,12 @@ const routeFiles = { test.describe("Error Sanitization", () => { let fixture: Fixture; let oldConsoleError: () => void; + let errorLogs: any[] = []; test.beforeEach(() => { oldConsoleError = console.error; - console.error = () => {}; + errorLogs = []; + console.error = (...args) => errorLogs.push(args); }); test.afterEach(() => { @@ -142,6 +146,11 @@ test.describe("Error Sanitization", () => { test.beforeAll(async () => { fixture = await createFixture( { + config: { + future: { + v2_errorBoundary: true, + }, + }, files: routeFiles, }, ServerMode.Production @@ -167,6 +176,9 @@ test.describe("Error Sanitization", () => { '{"routes/index":{"message":"Unexpected Server Error","__type":"Error"}}' ); expect(html).not.toMatch(/stack/i); + expect(errorLogs.length).toBe(1); + expect(errorLogs[0][0].message).toMatch("Loader Error"); + expect(errorLogs[0][0].stack).toMatch(" at "); }); test("sanitizes render errors in document requests", async () => { @@ -178,6 +190,9 @@ test.describe("Error Sanitization", () => { '{"routes/index":{"message":"Unexpected Server Error","__type":"Error"}}' ); expect(html).not.toMatch(/stack/i); + expect(errorLogs.length).toBe(1); + expect(errorLogs[0][0].message).toMatch("Render Error"); + expect(errorLogs[0][0].stack).toMatch(" at "); }); test("renders deferred document without errors", async () => { @@ -200,6 +215,9 @@ test.describe("Error Sanitization", () => { // Defer errors are not not part of the JSON blob but rather rejected // against a pending promise and therefore are inlined JS. expect(html).toMatch("x.stack=undefined;"); + // defer errors are not logged to the server console since the request + // has "succeeded" + expect(errorLogs.length).toBe(0); }); test("returns data without errors", async () => { @@ -214,6 +232,9 @@ test.describe("Error Sanitization", () => { let response = await fixture.requestData("/?loader", "routes/index"); let text = await response.text(); expect(text).toBe('{"message":"Unexpected Server Error"}'); + expect(errorLogs.length).toBe(1); + expect(errorLogs[0][0].message).toMatch("Loader Error"); + expect(errorLogs[0][0].stack).toMatch(" at "); }); test("returns deferred data without errors", async () => { @@ -231,6 +252,9 @@ test.describe("Error Sanitization", () => { '{"lazy":"__deferred_promise:lazy"}\n\n' + 'error:{"lazy":{"message":"Unexpected Server Error"}}\n\n' ); + // defer errors are not logged to the server console since the request + // has "succeeded" + expect(errorLogs.length).toBe(0); }); test("sanitizes loader errors in resource requests", async () => { @@ -240,12 +264,20 @@ test.describe("Error Sanitization", () => { ); let text = await response.text(); expect(text).toBe('{"message":"Unexpected Server Error"}'); + expect(errorLogs.length).toBe(1); + expect(errorLogs[0][0].message).toMatch("Loader Error"); + expect(errorLogs[0][0].stack).toMatch(" at "); }); test("sanitizes mismatched route errors in data requests", async () => { let response = await fixture.requestData("/", "not-a-route"); let text = await response.text(); expect(text).toBe('{"message":"Unexpected Server Error"}'); + expect(errorLogs.length).toBe(1); + expect(errorLogs[0][0].message).toMatch( + 'Route "not-a-route" does not match URL "/"' + ); + expect(errorLogs[0][0].stack).toMatch(" at "); }); }); @@ -253,6 +285,11 @@ test.describe("Error Sanitization", () => { test.beforeAll(async () => { fixture = await createFixture( { + config: { + future: { + v2_errorBoundary: true, + }, + }, files: routeFiles, }, ServerMode.Development @@ -286,6 +323,9 @@ test.describe("Error Sanitization", () => { expect(html).toMatch( 'errors":{"routes/index":{"message":"Loader Error","stack":"Error: Loader Error\\n' ); + expect(errorLogs.length).toBe(1); + expect(errorLogs[0][0].message).toMatch("Loader Error"); + expect(errorLogs[0][0].stack).toMatch(" at "); }); test("does not sanitize render errors in document requests", async () => { @@ -297,6 +337,9 @@ test.describe("Error Sanitization", () => { expect(html).toMatch( 'errors":{"routes/index":{"message":"Render Error","stack":"Error: Render Error\\n' ); + expect(errorLogs.length).toBe(1); + expect(errorLogs[0][0].message).toMatch("Render Error"); + expect(errorLogs[0][0].stack).toMatch(" at "); }); test("renders deferred document without errors", async () => { @@ -316,6 +359,9 @@ test.describe("Error Sanitization", () => { // Defer errors are not not part of the JSON blob but rather rejected // against a pending promise and therefore are inlined JS. expect(html).toMatch("x.stack=e.stack;"); + // defer errors are not logged to the server console since the request + // has "succeeded" + expect(errorLogs.length).toBe(0); }); test("returns data without errors", async () => { @@ -332,6 +378,9 @@ test.describe("Error Sanitization", () => { expect(text).toMatch( '{"message":"Loader Error","stack":"Error: Loader Error' ); + expect(errorLogs.length).toBe(1); + expect(errorLogs[0][0].message).toMatch("Loader Error"); + expect(errorLogs[0][0].stack).toMatch(" at "); }); test("returns deferred data without errors", async () => { @@ -348,6 +397,9 @@ test.describe("Error Sanitization", () => { expect(text).toMatch( 'error:{"lazy":{"message":"REJECTED","stack":"Error: REJECTED' ); + // defer errors are not logged to the server console since the request + // has "succeeded" + expect(errorLogs.length).toBe(0); }); test("does not sanitize loader errors in resource requests", async () => { @@ -359,6 +411,9 @@ test.describe("Error Sanitization", () => { expect(text).toMatch( '{"message":"Loader Error","stack":"Error: Loader Error' ); + expect(errorLogs.length).toBe(1); + expect(errorLogs[0][0].message).toMatch("Loader Error"); + expect(errorLogs[0][0].stack).toMatch(" at "); }); test("sanitizes mismatched route errors in data requests", async () => { @@ -367,6 +422,205 @@ test.describe("Error Sanitization", () => { expect(text).toMatch( '{"message":"Route \\"not-a-route\\" does not match URL \\"/\\"","stack":"Error: Route \\"not-a-route\\" does not match URL \\"/\\"' ); + expect(errorLogs.length).toBe(1); + expect(errorLogs[0][0].message).toMatch( + 'Route "not-a-route" does not match URL "/"' + ); + expect(errorLogs[0][0].stack).toMatch(" at "); + }); + }); + + test.describe("serverMode=production (user-provided onUnhandledError)", () => { + test.beforeAll(async () => { + fixture = await createFixture( + { + config: { + future: { + v2_errorBoundary: true, + }, + }, + files: { + "app/entry.server.tsx": js` + import type { EntryContext } from "@remix-run/node"; + import { RemixServer, isRouteErrorResponse } from "@remix-run/react"; + import { renderToString } from "react-dom/server"; + + export default function handleRequest( + request: Request, + responseStatusCode: number, + responseHeaders: Headers, + remixContext: EntryContext + ) { + let markup = renderToString( + + ); + + responseHeaders.set("Content-Type", "text/html"); + + return new Response("" + markup, { + status: responseStatusCode, + headers: responseHeaders, + }); + } + + export function onUnhandledError( + error: unknown, + { request }: { request: Request }, + ) { + console.error("App Specific Error Logging:"); + console.error(" Request: " + request.method + " " + request.url); + if (error instanceof Error) { + console.error(" Error: " + error.message); + console.error(" Stack: " + error.stack); + } else { + console.error("Dunno what this is"); + } + } + `, + ...routeFiles, + }, + }, + ServerMode.Production + ); + }); + + test("renders document without errors", async () => { + let response = await fixture.requestDocument("/"); + let html = await response.text(); + expect(html).toMatch("Index Route"); + expect(html).toMatch("LOADER"); + expect(html).not.toMatch("MESSAGE:"); + expect(html).not.toMatch(/stack/i); + }); + + test("sanitizes loader errors in document requests", async () => { + let response = await fixture.requestDocument("/?loader"); + let html = await response.text(); + expect(html).toMatch("Index Error"); + expect(html).not.toMatch("LOADER"); + expect(html).toMatch("MESSAGE:Unexpected Server Error"); + expect(html).toMatch( + '{"routes/index":{"message":"Unexpected Server Error","__type":"Error"}}' + ); + expect(html).not.toMatch(/stack/i); + expect(errorLogs[0][0]).toEqual("App Specific Error Logging:"); + expect(errorLogs[1][0]).toEqual(" Request: GET test://test/?loader"); + expect(errorLogs[2][0]).toEqual(" Error: Loader Error"); + expect(errorLogs[3][0]).toMatch(" at "); + expect(errorLogs.length).toBe(4); + }); + + test("sanitizes render errors in document requests", async () => { + let response = await fixture.requestDocument("/?render"); + let html = await response.text(); + expect(html).toMatch("Index Error"); + expect(html).toMatch("MESSAGE:Unexpected Server Error"); + expect(html).toMatch( + '{"routes/index":{"message":"Unexpected Server Error","__type":"Error"}}' + ); + expect(html).not.toMatch(/stack/i); + expect(errorLogs[0][0]).toEqual("App Specific Error Logging:"); + expect(errorLogs[1][0]).toEqual(" Request: GET test://test/?render"); + expect(errorLogs[2][0]).toEqual(" Error: Render Error"); + expect(errorLogs[3][0]).toMatch(" at "); + expect(errorLogs.length).toBe(4); + }); + + test("renders deferred document without errors", async () => { + let response = await fixture.requestDocument("/defer"); + let html = await response.text(); + expect(html).toMatch("Defer Route"); + expect(html).toMatch("RESOLVED"); + expect(html).not.toMatch("MESSAGE:"); + // Defer errors are not not part of the JSON blob but rather rejected + // against a pending promise and therefore are inlined JS. + expect(html).not.toMatch("x.stack=e.stack;"); + }); + + test("sanitizes defer errors in document requests", async () => { + let response = await fixture.requestDocument("/defer?loader"); + let html = await response.text(); + expect(html).toMatch("Defer Error"); + expect(html).not.toMatch("RESOLVED"); + expect(html).toMatch('{"message":"Unexpected Server Error"}'); + // Defer errors are not not part of the JSON blob but rather rejected + // against a pending promise and therefore are inlined JS. + expect(html).toMatch("x.stack=undefined;"); + // defer errors are not logged to the server console since the request + // has "succeeded" + expect(errorLogs.length).toBe(0); + }); + + test("returns data without errors", async () => { + let response = await fixture.requestData("/", "routes/index"); + let text = await response.text(); + expect(text).toMatch("LOADER"); + expect(text).not.toMatch("MESSAGE:"); + expect(text).not.toMatch(/stack/i); + }); + + test("sanitizes loader errors in data requests", async () => { + let response = await fixture.requestData("/?loader", "routes/index"); + let text = await response.text(); + expect(text).toBe('{"message":"Unexpected Server Error"}'); + expect(errorLogs[0][0]).toEqual("App Specific Error Logging:"); + expect(errorLogs[1][0]).toEqual( + " Request: GET test://test/?loader=&_data=routes%2Findex" + ); + expect(errorLogs[2][0]).toEqual(" Error: Loader Error"); + expect(errorLogs[3][0]).toMatch(" at "); + expect(errorLogs.length).toBe(4); + }); + + test("returns deferred data without errors", async () => { + let response = await fixture.requestData("/defer", "routes/defer"); + let text = await response.text(); + expect(text).toMatch("RESOLVED"); + expect(text).not.toMatch("REJECTED"); + expect(text).not.toMatch(/stack/i); + }); + + test("sanitizes loader errors in deferred data requests", async () => { + let response = await fixture.requestData("/defer?loader", "routes/defer"); + let text = await response.text(); + expect(text).toBe( + '{"lazy":"__deferred_promise:lazy"}\n\n' + + 'error:{"lazy":{"message":"Unexpected Server Error"}}\n\n' + ); + // defer errors are not logged to the server console since the request + // has "succeeded" + expect(errorLogs.length).toBe(0); + }); + + test("sanitizes loader errors in resource requests", async () => { + let response = await fixture.requestData( + "/resource?loader", + "routes/resource" + ); + let text = await response.text(); + expect(text).toBe('{"message":"Unexpected Server Error"}'); + expect(errorLogs[0][0]).toEqual("App Specific Error Logging:"); + expect(errorLogs[1][0]).toEqual( + " Request: GET test://test/resource?loader=&_data=routes%2Fresource" + ); + expect(errorLogs[2][0]).toEqual(" Error: Loader Error"); + expect(errorLogs[3][0]).toMatch(" at "); + expect(errorLogs.length).toBe(4); + }); + + test("sanitizes mismatched route errors in data requests", async () => { + let response = await fixture.requestData("/", "not-a-route"); + let text = await response.text(); + expect(text).toBe('{"message":"Unexpected Server Error"}'); + expect(errorLogs[0][0]).toEqual("App Specific Error Logging:"); + expect(errorLogs[1][0]).toEqual( + " Request: GET test://test/?_data=not-a-route" + ); + expect(errorLogs[2][0]).toEqual( + ' Error: Route "not-a-route" does not match URL "/"' + ); + expect(errorLogs[3][0]).toMatch(" at "); + expect(errorLogs.length).toBe(4); }); }); }); diff --git a/integration/headers-test.ts b/integration/headers-test.ts index 454c83ac319..a34dfe72854 100644 --- a/integration/headers-test.ts +++ b/integration/headers-test.ts @@ -1,4 +1,5 @@ import { test, expect } from "@playwright/test"; +import { ServerMode } from "@remix-run/server-runtime/mode"; import { createFixture, js } from "./helpers/create-fixture"; import type { Fixture } from "./helpers/create-fixture"; @@ -12,195 +13,198 @@ test.describe("headers export", () => { let appFixture: Fixture; test.beforeAll(async () => { - appFixture = await createFixture({ - config: { - future: { - v2_routeConvention: true, - v2_errorBoundary: true, - v2_headers: true, + appFixture = await createFixture( + { + config: { + future: { + v2_routeConvention: true, + v2_errorBoundary: true, + v2_headers: true, + }, }, - }, - files: { - "app/root.jsx": js` - import { json } from "@remix-run/node"; - import { Links, Meta, Outlet, Scripts } from "@remix-run/react"; - - export const loader = () => json({}); - - export default function Root() { - return ( - - - - - - - - - - - ); - } - `, - - "app/routes/_index.jsx": js` - import { json } from "@remix-run/node"; - - export function loader() { - return json(null, { - headers: { - "${ROOT_HEADER_KEY}": "${ROOT_HEADER_VALUE}" + files: { + "app/root.jsx": js` + import { json } from "@remix-run/node"; + import { Links, Meta, Outlet, Scripts } from "@remix-run/react"; + + export const loader = () => json({}); + + export default function Root() { + return ( + + + + + + + + + + + ); + } + `, + + "app/routes/_index.jsx": js` + import { json } from "@remix-run/node"; + + export function loader() { + return json(null, { + headers: { + "${ROOT_HEADER_KEY}": "${ROOT_HEADER_VALUE}" + } + }) + } + + export function headers({ loaderHeaders }) { + return { + "${ROOT_HEADER_KEY}": loaderHeaders.get("${ROOT_HEADER_KEY}") } - }) - } + } - export function headers({ loaderHeaders }) { - return { - "${ROOT_HEADER_KEY}": loaderHeaders.get("${ROOT_HEADER_KEY}") + export default function Index() { + return
Heyo!
} - } + `, - export default function Index() { - return
Heyo!
- } - `, + "app/routes/action.jsx": js` + import { json } from "@remix-run/node"; - "app/routes/action.jsx": js` - import { json } from "@remix-run/node"; + export function action() { + return json(null, { + headers: { + "${ACTION_HKEY}": "${ACTION_HVALUE}" + } + }) + } - export function action() { - return json(null, { - headers: { - "${ACTION_HKEY}": "${ACTION_HVALUE}" + export function headers({ actionHeaders }) { + return { + "${ACTION_HKEY}": actionHeaders.get("${ACTION_HKEY}") } - }) - } + } - export function headers({ actionHeaders }) { - return { - "${ACTION_HKEY}": actionHeaders.get("${ACTION_HKEY}") + export default function Action() { return
} + `, + + "app/routes/parent.jsx": js` + export function headers({ actionHeaders, errorHeaders, loaderHeaders, parentHeaders }) { + return new Headers([ + ...(parentHeaders ? Array.from(parentHeaders.entries()) : []), + ...(actionHeaders ? Array.from(actionHeaders.entries()) : []), + ...(loaderHeaders ? Array.from(loaderHeaders.entries()) : []), + ...(errorHeaders ? Array.from(errorHeaders.entries()) : []), + ]); } - } - - export default function Action() { return
} - `, - - "app/routes/parent.jsx": js` - export function headers({ actionHeaders, errorHeaders, loaderHeaders, parentHeaders }) { - return new Headers([ - ...(parentHeaders ? Array.from(parentHeaders.entries()) : []), - ...(actionHeaders ? Array.from(actionHeaders.entries()) : []), - ...(loaderHeaders ? Array.from(loaderHeaders.entries()) : []), - ...(errorHeaders ? Array.from(errorHeaders.entries()) : []), - ]); - } - - export function loader({ request }) { - if (new URL(request.url).searchParams.get('throw') === "parent") { - throw new Response(null, { - status: 400, - headers: { 'X-Parent-Loader': 'error' }, + + export function loader({ request }) { + if (new URL(request.url).searchParams.get('throw') === "parent") { + throw new Response(null, { + status: 400, + headers: { 'X-Parent-Loader': 'error' }, + }) + } + return new Response(null, { + headers: { 'X-Parent-Loader': 'success' }, }) } - return new Response(null, { - headers: { 'X-Parent-Loader': 'success' }, - }) - } - - export async function action({ request }) { - let fd = await request.formData(); - if (fd.get('throw') === "parent") { - throw new Response(null, { - status: 400, - headers: { 'X-Parent-Action': 'error' }, + + export async function action({ request }) { + let fd = await request.formData(); + if (fd.get('throw') === "parent") { + throw new Response(null, { + status: 400, + headers: { 'X-Parent-Action': 'error' }, + }) + } + return new Response(null, { + headers: { 'X-Parent-Action': 'success' }, }) } - return new Response(null, { - headers: { 'X-Parent-Action': 'success' }, - }) - } - export default function Component() { return
} + export default function Component() { return
} - export function ErrorBoundary() { - return

Error!

- } - `, + export function ErrorBoundary() { + return

Error!

+ } + `, + + "app/routes/parent.child.jsx": js` + export function loader({ request }) { + if (new URL(request.url).searchParams.get('throw') === "child") { + throw new Response(null, { + status: 400, + headers: { 'X-Child-Loader': 'error' }, + }) + } + return null + } - "app/routes/parent.child.jsx": js` - export function loader({ request }) { - if (new URL(request.url).searchParams.get('throw') === "child") { - throw new Response(null, { - status: 400, - headers: { 'X-Child-Loader': 'error' }, - }) + export async function action({ request }) { + let fd = await request.formData(); + if (fd.get('throw') === "child") { + throw new Response(null, { + status: 400, + headers: { 'X-Child-Action': 'error' }, + }) + } + return null } - return null - } - export async function action({ request }) { - let fd = await request.formData(); - if (fd.get('throw') === "child") { + export default function Component() { return
} + `, + + "app/routes/parent.child.grandchild.jsx": js` + export function loader({ request }) { throw new Response(null, { status: 400, - headers: { 'X-Child-Action': 'error' }, + headers: { 'X-Child-Grandchild': 'error' }, }) } - return null - } - - export default function Component() { return
} - `, - - "app/routes/parent.child.grandchild.jsx": js` - export function loader({ request }) { - throw new Response(null, { - status: 400, - headers: { 'X-Child-Grandchild': 'error' }, - }) - } - - export default function Component() { return
} - `, - - "app/routes/cookie.jsx": js` - import { json } from "@remix-run/server-runtime"; - import { Outlet } from "@remix-run/react"; - - export function loader({ request }) { - if (new URL(request.url).searchParams.has("parent-throw")) { - throw json(null, { headers: { "Set-Cookie": "parent-thrown-cookie=true" } }); + + export default function Component() { return
} + `, + + "app/routes/cookie.jsx": js` + import { json } from "@remix-run/server-runtime"; + import { Outlet } from "@remix-run/react"; + + export function loader({ request }) { + if (new URL(request.url).searchParams.has("parent-throw")) { + throw json(null, { headers: { "Set-Cookie": "parent-thrown-cookie=true" } }); + } + return null + }; + + export default function Parent() { + return ; } - return null - }; - export default function Parent() { - return ; - } + export function ErrorBoundary() { + return

Caught!

; + } + `, - export function ErrorBoundary() { - return

Caught!

; - } - `, + "app/routes/cookie.child.jsx": js` + import { json } from "@remix-run/node"; - "app/routes/cookie.child.jsx": js` - import { json } from "@remix-run/node"; + export function loader({ request }) { + if (new URL(request.url).searchParams.has("throw")) { + throw json(null, { headers: { "Set-Cookie": "thrown-cookie=true" } }); + } + return json(null, { + headers: { "Set-Cookie": "normal-cookie=true" }, + }); + }; - export function loader({ request }) { - if (new URL(request.url).searchParams.has("throw")) { - throw json(null, { headers: { "Set-Cookie": "thrown-cookie=true" } }); + export default function Child() { + return

Child

; } - return json(null, { - headers: { "Set-Cookie": "normal-cookie=true" }, - }); - }; - - export default function Child() { - return

Child

; - } - `, + `, + }, }, - }); + ServerMode.Test + ); }); test("can use `action` headers", async () => { @@ -220,53 +224,56 @@ test.describe("headers export", () => { let HEADER_KEY = "X-Test"; let HEADER_VALUE = "SUCCESS"; - let fixture = await createFixture({ - config: { - future: { v2_routeConvention: true }, - }, - files: { - "app/root.jsx": js` - import { Links, Meta, Outlet, Scripts } from "@remix-run/react"; - - export default function Root() { - return ( - - - - - - - - - - - ); - } - `, - - "app/routes/_index.jsx": js` - import { json } from "@remix-run/node"; - - export function loader() { - return json(null, { - headers: { - "${HEADER_KEY}": "${HEADER_VALUE}" - } - }) - } + let fixture = await createFixture( + { + config: { + future: { v2_routeConvention: true }, + }, + files: { + "app/root.jsx": js` + import { Links, Meta, Outlet, Scripts } from "@remix-run/react"; + + export default function Root() { + return ( + + + + + + + + + + + ); + } + `, - export function headers({ loaderHeaders }) { - return { - "${HEADER_KEY}": loaderHeaders.get("${HEADER_KEY}") + "app/routes/_index.jsx": js` + import { json } from "@remix-run/node"; + + export function loader() { + return json(null, { + headers: { + "${HEADER_KEY}": "${HEADER_VALUE}" + } + }) } - } - export default function Index() { - return
Heyo!
- } - `, + export function headers({ loaderHeaders }) { + return { + "${HEADER_KEY}": loaderHeaders.get("${HEADER_KEY}") + } + } + + export default function Index() { + return
Heyo!
+ } + `, + }, }, - }); + ServerMode.Test + ); let response = await fixture.requestDocument("/"); expect(response.headers.get(HEADER_KEY)).toBe(HEADER_VALUE); }); @@ -427,102 +434,105 @@ test.describe("v1 behavior (future.v2_headers=false)", () => { let appFixture: Fixture; test.beforeAll(async () => { - appFixture = await createFixture({ - config: { - future: { - v2_routeConvention: true, - v2_errorBoundary: true, - v2_headers: false, + appFixture = await createFixture( + { + config: { + future: { + v2_routeConvention: true, + v2_errorBoundary: true, + v2_headers: false, + }, }, - }, - files: { - "app/root.jsx": js` - import { json } from "@remix-run/node"; - import { Links, Meta, Outlet, Scripts } from "@remix-run/react"; - - export const loader = () => json({}); - - export default function Root() { - return ( - - - - - - - - - - - ); - } - `, - - "app/routes/parent.jsx": js` - export function headers({ actionHeaders, errorHeaders, loaderHeaders, parentHeaders }) { - return new Headers([ - ...(parentHeaders ? Array.from(parentHeaders.entries()) : []), - ...(actionHeaders ? Array.from(actionHeaders.entries()) : []), - ...(loaderHeaders ? Array.from(loaderHeaders.entries()) : []), - ...(errorHeaders ? Array.from(errorHeaders.entries()) : []), - ]); - } - - export function loader({ request }) { - return new Response(null, { - headers: { 'X-Parent-Loader': 'success' }, - }) - } - - export default function Component() { return
} - `, - - "app/routes/parent.child.jsx": js` - export async function action({ request }) { - return null; - } - - export default function Component() { return
} - `, - - "app/routes/cookie.jsx": js` - import { json } from "@remix-run/server-runtime"; - import { Outlet } from "@remix-run/react"; - - export function loader({ request }) { - if (new URL(request.url).searchParams.has("parent-throw")) { - throw json(null, { headers: { "Set-Cookie": "parent-thrown-cookie=true" } }); + files: { + "app/root.jsx": js` + import { json } from "@remix-run/node"; + import { Links, Meta, Outlet, Scripts } from "@remix-run/react"; + + export const loader = () => json({}); + + export default function Root() { + return ( + + + + + + + + + + + ); + } + `, + + "app/routes/parent.jsx": js` + export function headers({ actionHeaders, errorHeaders, loaderHeaders, parentHeaders }) { + return new Headers([ + ...(parentHeaders ? Array.from(parentHeaders.entries()) : []), + ...(actionHeaders ? Array.from(actionHeaders.entries()) : []), + ...(loaderHeaders ? Array.from(loaderHeaders.entries()) : []), + ...(errorHeaders ? Array.from(errorHeaders.entries()) : []), + ]); } - return null - }; - export default function Parent() { - return ; - } + export function loader({ request }) { + return new Response(null, { + headers: { 'X-Parent-Loader': 'success' }, + }) + } + + export default function Component() { return
} + `, + + "app/routes/parent.child.jsx": js` + export async function action({ request }) { + return null; + } + + export default function Component() { return
} + `, + + "app/routes/cookie.jsx": js` + import { json } from "@remix-run/server-runtime"; + import { Outlet } from "@remix-run/react"; + + export function loader({ request }) { + if (new URL(request.url).searchParams.has("parent-throw")) { + throw json(null, { headers: { "Set-Cookie": "parent-thrown-cookie=true" } }); + } + return null + }; + + export default function Parent() { + return ; + } + + export function ErrorBoundary() { + return

Caught!

; + } + `, - export function ErrorBoundary() { - return

Caught!

; - } - `, + "app/routes/cookie.child.jsx": js` + import { json } from "@remix-run/node"; - "app/routes/cookie.child.jsx": js` - import { json } from "@remix-run/node"; + export function loader({ request }) { + if (new URL(request.url).searchParams.has("throw")) { + throw json(null, { headers: { "Set-Cookie": "thrown-cookie=true" } }); + } + return json(null, { + headers: { "Set-Cookie": "normal-cookie=true" }, + }); + }; - export function loader({ request }) { - if (new URL(request.url).searchParams.has("throw")) { - throw json(null, { headers: { "Set-Cookie": "thrown-cookie=true" } }); + export default function Child() { + return

Child

; } - return json(null, { - headers: { "Set-Cookie": "normal-cookie=true" }, - }); - }; - - export default function Child() { - return

Child

; - } - `, + `, + }, }, - }); + ServerMode.Test + ); }); test("returns no headers when the leaf route doesn't export a header function (GET)", async () => { diff --git a/packages/remix-cloudflare/index.ts b/packages/remix-cloudflare/index.ts index e5850c1acb6..9d27898e2e4 100644 --- a/packages/remix-cloudflare/index.ts +++ b/packages/remix-cloudflare/index.ts @@ -68,6 +68,7 @@ export type { MemoryUploadHandlerOptions, MetaDescriptor, MetaFunction, + OnUnhandledErrorFunction, PageLinkDescriptor, RequestHandler, RouteComponent, diff --git a/packages/remix-deno/index.ts b/packages/remix-deno/index.ts index 2eee795dfd6..a4ec61f2d34 100644 --- a/packages/remix-deno/index.ts +++ b/packages/remix-deno/index.ts @@ -56,6 +56,7 @@ export type { MemoryUploadHandlerOptions, MetaDescriptor, MetaFunction, + OnUnhandledErrorFunction, PageLinkDescriptor, RequestHandler, RouteComponent, diff --git a/packages/remix-dev/codemod/replace-remix-magic-imports/utils/export.ts b/packages/remix-dev/codemod/replace-remix-magic-imports/utils/export.ts index 5be1b002c6f..26b0e315398 100644 --- a/packages/remix-dev/codemod/replace-remix-magic-imports/utils/export.ts +++ b/packages/remix-dev/codemod/replace-remix-magic-imports/utils/export.ts @@ -68,6 +68,7 @@ const defaultRuntimeExports: ExportNames = { "MemoryUploadHandlerOptions", "MetaDescriptor", "MetaFunction", + "OnUnhandledErrorFunction", "PageLinkDescriptor", "RequestHandler", "RouteComponent", diff --git a/packages/remix-dev/config/defaults/cloudflare/entry.server.react-stream.tsx b/packages/remix-dev/config/defaults/cloudflare/entry.server.react-stream.tsx index 3e08f46fe53..a393a5228ea 100644 --- a/packages/remix-dev/config/defaults/cloudflare/entry.server.react-stream.tsx +++ b/packages/remix-dev/config/defaults/cloudflare/entry.server.react-stream.tsx @@ -14,6 +14,7 @@ export default async function handleRequest( { signal: request.signal, onError(error: unknown) { + // Log streaming rendering errors from inside the shell console.error(error); responseStatusCode = 500; }, diff --git a/packages/remix-dev/config/defaults/deno/entry.server.react-stream.tsx b/packages/remix-dev/config/defaults/deno/entry.server.react-stream.tsx index 7be6c748061..91a1d11e52b 100644 --- a/packages/remix-dev/config/defaults/deno/entry.server.react-stream.tsx +++ b/packages/remix-dev/config/defaults/deno/entry.server.react-stream.tsx @@ -14,6 +14,7 @@ export default async function handleRequest( { signal: request.signal, onError(error: unknown) { + // Log streaming rendering errors from inside the shell console.error(error); responseStatusCode = 500; }, diff --git a/packages/remix-dev/config/defaults/node/entry.server.react-stream.tsx b/packages/remix-dev/config/defaults/node/entry.server.react-stream.tsx index e9f6ac9846a..efe28dbcfb9 100644 --- a/packages/remix-dev/config/defaults/node/entry.server.react-stream.tsx +++ b/packages/remix-dev/config/defaults/node/entry.server.react-stream.tsx @@ -36,6 +36,7 @@ function handleBotRequest( remixContext: EntryContext ) { return new Promise((resolve, reject) => { + let shellRendered = false; const { pipe, abort } = renderToPipeableStream( , { onAllReady() { + shellRendered = true; const body = new PassThrough(); responseHeaders.set("Content-Type", "text/html"); @@ -62,7 +64,12 @@ function handleBotRequest( }, onError(error: unknown) { responseStatusCode = 500; - console.error(error); + // Log streaming rendering errors from inside the shell. Don't log + // errors encountered during initial shell rendering since they'll + // reject and get logged in handleDocumentRequest. + if (shellRendered) { + console.error(error); + } }, } ); @@ -78,6 +85,7 @@ function handleBrowserRequest( remixContext: EntryContext ) { return new Promise((resolve, reject) => { + let shellRendered = false; const { pipe, abort } = renderToPipeableStream( , { onShellReady() { + shellRendered = true; const body = new PassThrough(); responseHeaders.set("Content-Type", "text/html"); @@ -103,8 +112,13 @@ function handleBrowserRequest( reject(error); }, onError(error: unknown) { - console.error(error); responseStatusCode = 500; + // Log streaming rendering errors from inside the shell. Don't log + // errors encountered during initial shell rendering since they'll + // reject and get logged in handleDocumentRequest. + if (shellRendered) { + console.error(error); + } }, } ); diff --git a/packages/remix-eslint-config/rules/packageExports.js b/packages/remix-eslint-config/rules/packageExports.js index b3bfa1a3f94..49892da0e93 100644 --- a/packages/remix-eslint-config/rules/packageExports.js +++ b/packages/remix-eslint-config/rules/packageExports.js @@ -46,6 +46,7 @@ const defaultRuntimeExports = { "MemoryUploadHandlerOptions", "MetaDescriptor", "MetaFunction", + "OnUnhandledErrorFunction", "PageLinkDescriptor", "RequestHandler", "RouteComponent", diff --git a/packages/remix-node/index.ts b/packages/remix-node/index.ts index 8e0d62dbeeb..20fa129bb1b 100644 --- a/packages/remix-node/index.ts +++ b/packages/remix-node/index.ts @@ -79,6 +79,7 @@ export type { MemoryUploadHandlerOptions, MetaDescriptor, MetaFunction, + OnUnhandledErrorFunction, PageLinkDescriptor, RequestHandler, RouteComponent, diff --git a/packages/remix-react/errorBoundaries.tsx b/packages/remix-react/errorBoundaries.tsx index 32eb75b4e1d..24d63344aed 100644 --- a/packages/remix-react/errorBoundaries.tsx +++ b/packages/remix-react/errorBoundaries.tsx @@ -70,7 +70,10 @@ export class RemixErrorBoundary extends React.Component< * When app's don't provide a root level ErrorBoundary, we default to this. */ export function RemixRootDefaultErrorBoundary({ error }: { error: Error }) { - console.error(error); + // Only log client side to avoid double-logging on the server + React.useEffect(() => { + console.error(error); + }, [error]); return ( @@ -84,16 +87,18 @@ export function RemixRootDefaultErrorBoundary({ error }: { error: Error }) {

Application Error

- 
-            {error.stack}
-
+ {error.stack ? ( + 
+              {error.stack}
+
+ ) : null}