From 47ad139385e95b1083ecac4ac2f0ff0ecbfd8d65 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 5 Jan 2023 14:33:21 -0500 Subject: [PATCH 1/6] Add outer RemixErrorBoundary to catch root boundary-thrown errors --- integration/error-boundary-test.ts | 317 +++++++++++++++++++++++++++++ packages/remix-react/browser.tsx | 23 ++- packages/remix-react/package.json | 3 +- packages/remix-react/server.tsx | 19 +- yarn.lock | 5 + 5 files changed, 360 insertions(+), 7 deletions(-) diff --git a/integration/error-boundary-test.ts b/integration/error-boundary-test.ts index d567effab31..830d58c2e19 100644 --- a/integration/error-boundary-test.ts +++ b/integration/error-boundary-test.ts @@ -875,3 +875,320 @@ test.describe("loaderData in ErrorBoundary", () => { }); } }); + +test.describe("Default ErrorBoundary", () => { + let fixture: Fixture; + let appFixture: AppFixture; + let _consoleError: any; + + function getFiles({ + includeRootErrorBoundary = false, + rootErrorBoundaryThrows = false, + } = {}) { + let errorBoundaryCode = !includeRootErrorBoundary + ? "" + : rootErrorBoundaryThrows + ? js` + export function ErrorBoundary({ error }) { + return ( + + + +
+
Root Error Boundary
+

{error.message}

+

{oh.no.what.have.i.done}

+
+ + + + ) + } + ` + : js` + export function ErrorBoundary({ error }) { + return ( + + + +
+
Root Error Boundary
+

{error.message}

+
+ + + + ) + } + `; + + return { + "app/root.jsx": js` + import { Links, Meta, Outlet, Scripts } from "@remix-run/react"; + + export default function Root() { + return ( + + + + + + +
+ +
+ + + + ); + } + + ${errorBoundaryCode} + `, + + "app/routes/index.jsx": js` + import { Link } from "@remix-run/react"; + export default function () { + return ( +
+

Index

+ Loader Error + Render Error +
+ ); + } + `, + + "app/routes/loader-error.jsx": js` + export function loader() { + throw new Error('Loader Error'); + } + export default function () { + return

Loader Error

+ } + `, + + "app/routes/render-error.jsx": js` + export default function () { + throw new Error("Render Error") + } + `, + }; + } + + test.beforeAll(async () => { + _consoleError = console.error; + console.error = () => {}; + }); + + test.afterAll(async () => { + console.error = _consoleError; + await appFixture.close(); + }); + + test.describe("When the root route does not have a boundary", () => { + test.beforeAll(async () => { + fixture = await createFixture({ + files: getFiles({ includeRootErrorBoundary: false }), + }); + appFixture = await createAppFixture(fixture, ServerMode.Development); + }); + + test.afterAll(async () => { + await appFixture.close(); + }); + + test.describe("document requests", () => { + test("renders default boundary on loader errors", async () => { + let res = await fixture.requestDocument("/loader-error"); + expect(res.status).toBe(500); + let text = await res.text(); + expect(text).toMatch("Application Error"); + expect(text).toMatch("Loader Error"); + expect(text).not.toMatch("Root Error Boundary"); + }); + + test("renders default boundary on render errors", async () => { + let res = await fixture.requestDocument("/render-error"); + expect(res.status).toBe(500); + let text = await res.text(); + expect(text).toMatch("Application Error"); + expect(text).toMatch("Render Error"); + expect(text).not.toMatch("Root Error Boundary"); + }); + }); + + test.describe("SPA navigations", () => { + test("renders default boundary on loader errors", async ({ page }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/"); + await app.clickLink("/loader-error"); + await page.waitForSelector("pre"); + let html = await app.getHtml(); + expect(html).toMatch("Application Error"); + expect(html).toMatch("Loader Error"); + expect(html).not.toMatch("Root Error Boundary"); + + // Ensure we can click back to our prior page + await app.goBack(); + await page.waitForSelector("h1#index"); + }); + + test("renders default boundary on render errors", async ({ page }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/"); + await app.clickLink("/render-error"); + await page.waitForSelector("pre"); + let html = await app.getHtml(); + expect(html).toMatch("Application Error"); + expect(html).toMatch("Render Error"); + expect(html).not.toMatch("Root Error Boundary"); + + // Ensure we can click back to our prior page + await app.goBack(); + await page.waitForSelector("h1#index"); + }); + }); + }); + + test.describe("When the root route has a boundary", () => { + test.beforeAll(async () => { + fixture = await createFixture({ + files: getFiles({ includeRootErrorBoundary: true }), + }); + appFixture = await createAppFixture(fixture, ServerMode.Development); + }); + + test.afterAll(async () => { + await appFixture.close(); + }); + + test.describe("document requests", () => { + test("renders root boundary on loader errors", async () => { + let res = await fixture.requestDocument("/loader-error"); + expect(res.status).toBe(500); + let text = await res.text(); + expect(text).toMatch("Root Error Boundary"); + expect(text).toMatch("Loader Error"); + expect(text).not.toMatch("Application Error"); + }); + + test("renders root boundary on render errors", async () => { + let res = await fixture.requestDocument("/render-error"); + expect(res.status).toBe(500); + let text = await res.text(); + expect(text).toMatch("Root Error Boundary"); + expect(text).toMatch("Render Error"); + expect(text).not.toMatch("Application Error"); + }); + }); + + test.describe("SPA navigations", () => { + test("renders root boundary on loader errors", async ({ page }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/"); + await app.clickLink("/loader-error"); + await page.waitForSelector("#root-error-boundary"); + let html = await app.getHtml(); + expect(html).toMatch("Root Error Boundary"); + expect(html).toMatch("Loader Error"); + expect(html).not.toMatch("Application Error"); + + // Ensure we can click back to our prior page + await app.goBack(); + await page.waitForSelector("h1#index"); + }); + + test("renders root boundary on render errors", async ({ page }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/"); + await app.clickLink("/render-error"); + await page.waitForSelector("#root-error-boundary"); + let html = await app.getHtml(); + expect(html).toMatch("Root Error Boundary"); + expect(html).toMatch("Render Error"); + expect(html).not.toMatch("Application Error"); + + // Ensure we can click back to our prior page + await app.goBack(); + await page.waitForSelector("h1#index"); + }); + }); + }); + + test.describe("When the root route has a boundary but it also throws 😦", () => { + test.beforeAll(async () => { + fixture = await createFixture({ + files: getFiles({ + includeRootErrorBoundary: true, + rootErrorBoundaryThrows: true, + }), + }); + appFixture = await createAppFixture(fixture, ServerMode.Development); + }); + + test.afterAll(async () => { + await appFixture.close(); + }); + + test.describe("document requests", () => { + test("tries to render root boundary on loader errors but bubbles to default boundary", async () => { + let res = await fixture.requestDocument("/loader-error"); + expect(res.status).toBe(500); + let text = await res.text(); + expect(text).toMatch("Unexpected Server Error"); + expect(text).not.toMatch("Application Error"); + expect(text).not.toMatch("Loader Error"); + expect(text).not.toMatch("Root Error Boundary"); + }); + + test("tries to render root boundary on render errors but bubbles to default boundary", async () => { + let res = await fixture.requestDocument("/render-error"); + expect(res.status).toBe(500); + let text = await res.text(); + expect(text).toMatch("Unexpected Server Error"); + expect(text).not.toMatch("Application Error"); + expect(text).not.toMatch("Render Error"); + expect(text).not.toMatch("Root Error Boundary"); + }); + }); + + test.describe("SPA navigations", () => { + test("tries to render root boundary on loader errors but bubbles to default boundary", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/"); + await app.clickLink("/loader-error"); + await page.waitForSelector("pre"); + let html = await app.getHtml(); + expect(html).toMatch("Application Error"); + expect(html).toMatch("ReferenceError: oh is not defined"); + expect(html).not.toMatch("Loader Error"); + expect(html).not.toMatch("Root Error Boundary"); + + // Ensure we can click back to our prior page + await app.goBack(); + await page.waitForSelector("h1#index"); + }); + + test("tries to render root boundary on render errors but bubbles to default boundary", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/"); + await app.clickLink("/render-error"); + await page.waitForSelector("pre"); + let html = await app.getHtml(); + expect(html).toMatch("Application Error"); + expect(html).toMatch("ReferenceError: oh is not defined"); + expect(html).not.toMatch("Render Error"); + expect(html).not.toMatch("Root Error Boundary"); + + // Ensure we can click back to our prior page + await app.goBack(); + await page.waitForSelector("h1#index"); + }); + }); + }); +}); diff --git a/packages/remix-react/browser.tsx b/packages/remix-react/browser.tsx index e2993283335..c4b43a492a9 100644 --- a/packages/remix-react/browser.tsx +++ b/packages/remix-react/browser.tsx @@ -1,10 +1,16 @@ import type { HydrationState, Router } from "@remix-run/router"; import type { ReactElement } from "react"; import * as React from "react"; +import type { Location } from "react-router-dom"; import { createBrowserRouter, RouterProvider } from "react-router-dom"; +import { useSyncExternalStore } from "use-sync-external-store/shim"; import { RemixContext } from "./components"; import type { EntryContext, FutureConfig } from "./entry"; +import { + RemixErrorBoundary, + RemixRootDefaultErrorBoundary, +} from "./errorBoundaries"; import { deserializeErrors } from "./errors"; import type { RouteModules } from "./routeModules"; import { createClientRoutes } from "./routes"; @@ -47,6 +53,16 @@ export function RemixBrowser(_props: RemixBrowserProps): ReactElement { router = createBrowserRouter(routes, { hydrationData }); } + // We need to include a wrapper RemixErrorBoundary here in case the root error + // boundary also throws and we need to bubble up outside of the router entirely. + // Then we need a stateful location here so the user can back0-button navigate + // out of there + let location: Location = useSyncExternalStore( + router.subscribe, + () => router.state.location, + () => router.state.location + ); + return ( - + + + ); } diff --git a/packages/remix-react/package.json b/packages/remix-react/package.json index 147c39a7011..98dc2ef80c6 100644 --- a/packages/remix-react/package.json +++ b/packages/remix-react/package.json @@ -17,7 +17,8 @@ "module": "dist/esm/index.js", "dependencies": { "@remix-run/router": "1.2.1", - "react-router-dom": "6.6.2-pre.0" + "react-router-dom": "6.6.2-pre.0", + "use-sync-external-store": "1.2.0" }, "devDependencies": { "@remix-run/server-runtime": "1.10.0-pre.6", diff --git a/packages/remix-react/server.tsx b/packages/remix-react/server.tsx index 2525ee24945..61bc6816c47 100644 --- a/packages/remix-react/server.tsx +++ b/packages/remix-react/server.tsx @@ -7,6 +7,10 @@ import { import { RemixContext } from "./components"; import type { EntryContext } from "./entry"; +import { + RemixErrorBoundary, + RemixRootDefaultErrorBoundary, +} from "./errorBoundaries"; import { createServerRoutes } from "./routes"; export interface RemixServerProps { @@ -37,11 +41,16 @@ export function RemixServer({ context, url }: RemixServerProps): ReactElement { future: context.future, }} > - + + + ); } diff --git a/yarn.lock b/yarn.lock index 147f52b0c3c..90729928cfe 100644 --- a/yarn.lock +++ b/yarn.lock @@ -12509,6 +12509,11 @@ url@0.10.3: punycode "1.3.2" querystring "0.2.0" +use-sync-external-store@1.2.0: + version "1.2.0" + resolved "https://registry.npmjs.org/use-sync-external-store/-/use-sync-external-store-1.2.0.tgz#7dbefd6ef3fe4e767a0cf5d7287aacfb5846928a" + integrity sha512-eEgnFxGQ1Ife9bzYs6VLi8/4X6CObHMw9Qr9tPY43iKwsPw8xE8+EFsf/2cFZ5S3esXgpWgtSCtLNS41F+sKPA== + use@^3.1.0: version "3.1.1" resolved "https://registry.npmjs.org/use/-/use-3.1.1.tgz" From 026df02b45288eaacd057f39cab257b21a570bb6 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 5 Jan 2023 15:34:44 -0500 Subject: [PATCH 2/6] Handle firefox not including error message in stack --- integration/error-boundary-test.ts | 21 +++++++++++++++------ packages/remix-server-runtime/server.ts | 2 +- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/integration/error-boundary-test.ts b/integration/error-boundary-test.ts index 830d58c2e19..e660a20ff04 100644 --- a/integration/error-boundary-test.ts +++ b/integration/error-boundary-test.ts @@ -1034,14 +1034,19 @@ test.describe("Default ErrorBoundary", () => { await page.waitForSelector("h1#index"); }); - test("renders default boundary on render errors", async ({ page }) => { + test("renders default boundary on render errors", async ({ + page, + }, workerInfo) => { let app = new PlaywrightFixture(appFixture, page); await app.goto("/"); await app.clickLink("/render-error"); await page.waitForSelector("pre"); let html = await app.getHtml(); expect(html).toMatch("Application Error"); - expect(html).toMatch("Render Error"); + // Chromium seems to be the only one that includes the message in the stack + if (workerInfo.project.name === "chromium") { + expect(html).toMatch("Render Error"); + } expect(html).not.toMatch("Root Error Boundary"); // Ensure we can click back to our prior page @@ -1156,14 +1161,16 @@ test.describe("Default ErrorBoundary", () => { test.describe("SPA navigations", () => { test("tries to render root boundary on loader errors but bubbles to default boundary", async ({ page, - }) => { + }, workerInfo) => { let app = new PlaywrightFixture(appFixture, page); await app.goto("/"); await app.clickLink("/loader-error"); await page.waitForSelector("pre"); let html = await app.getHtml(); expect(html).toMatch("Application Error"); - expect(html).toMatch("ReferenceError: oh is not defined"); + if (workerInfo.project.name === "chromium") { + expect(html).toMatch("ReferenceError: oh is not defined"); + } expect(html).not.toMatch("Loader Error"); expect(html).not.toMatch("Root Error Boundary"); @@ -1174,14 +1181,16 @@ test.describe("Default ErrorBoundary", () => { test("tries to render root boundary on render errors but bubbles to default boundary", async ({ page, - }) => { + }, workerInfo) => { let app = new PlaywrightFixture(appFixture, page); await app.goto("/"); await app.clickLink("/render-error"); await page.waitForSelector("pre"); let html = await app.getHtml(); expect(html).toMatch("Application Error"); - expect(html).toMatch("ReferenceError: oh is not defined"); + if (workerInfo.project.name === "chromium") { + expect(html).toMatch("ReferenceError: oh is not defined"); + } expect(html).not.toMatch("Render Error"); expect(html).not.toMatch("Root Error Boundary"); diff --git a/packages/remix-server-runtime/server.ts b/packages/remix-server-runtime/server.ts index 449a685e4a5..cdbaaad1a94 100644 --- a/packages/remix-server-runtime/server.ts +++ b/packages/remix-server-runtime/server.ts @@ -259,7 +259,7 @@ async function handleDocumentRequestRR( ); } catch (error: unknown) { if (serverMode !== ServerMode.Test) { - console.log("Error in entry.server handleDocumentRequest:", error); + console.error("Error in entry.server handleDocumentRequest:", error); } // Get a new StaticHandlerContext that contains the error at the right boundary From 3d4c16306d7ee5f1c76ce16f65cec87fcddc4183 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 5 Jan 2023 15:46:03 -0500 Subject: [PATCH 3/6] add types for useSyncExternalStore --- package.json | 1 + yarn.lock | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/package.json b/package.json index 071dac997ff..239b1e16035 100644 --- a/package.json +++ b/package.json @@ -83,6 +83,7 @@ "@types/retry": "^0.12.0", "@types/semver": "^7.3.4", "@types/ssri": "^7.1.0", + "@types/use-sync-external-store": "^0.0.3", "abort-controller": "^3.0.0", "abortcontroller-polyfill": "^1.7.3", "aws-sdk": "^2.1055.0", diff --git a/yarn.lock b/yarn.lock index 90729928cfe..69b20e927dd 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3004,6 +3004,11 @@ resolved "https://registry.npmjs.org/@types/unist/-/unist-2.0.6.tgz" integrity sha512-PBjIUxZHOuj0R15/xuwJYjFi+KZdNFrehocChv4g5hu6aFroHue8m0lBP0POdK2nKzbw0cgV1mws8+V/JAcEkQ== +"@types/use-sync-external-store@^0.0.3": + version "0.0.3" + resolved "https://registry.npmjs.org/@types/use-sync-external-store/-/use-sync-external-store-0.0.3.tgz#b6725d5f4af24ace33b36fafd295136e75509f43" + integrity sha512-EwmlvuaxPNej9+T4v5AuBPJa2x2UOJVdjCtDHgcDqitUeOtjnJKJ+apYjVcAoBEMjKW1VVFGZLUb5+qqa09XFA== + "@types/ws@^7.4.1": version "7.4.7" resolved "https://registry.npmjs.org/@types/ws/-/ws-7.4.7.tgz" From d2f35cfdde2e6220a4eeddfc315c4c5cd1d39500 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 5 Jan 2023 15:58:27 -0500 Subject: [PATCH 4/6] Remove duplicate error log --- packages/remix-server-runtime/server.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/remix-server-runtime/server.ts b/packages/remix-server-runtime/server.ts index cdbaaad1a94..7c0142760cc 100644 --- a/packages/remix-server-runtime/server.ts +++ b/packages/remix-server-runtime/server.ts @@ -258,10 +258,6 @@ async function handleDocumentRequestRR( entryContext ); } catch (error: unknown) { - if (serverMode !== ServerMode.Test) { - console.error("Error in entry.server handleDocumentRequest:", error); - } - // Get a new StaticHandlerContext that contains the error at the right boundary context = getStaticContextFromError( staticHandler.dataRoutes, From edc141d1a4e30e7b1ac5a5dfe07a0710a906e693 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 6 Jan 2023 10:26:17 -0500 Subject: [PATCH 5/6] fix typo Co-authored-by: Jacob Ebey --- packages/remix-react/browser.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/remix-react/browser.tsx b/packages/remix-react/browser.tsx index c4b43a492a9..ce280c23ca2 100644 --- a/packages/remix-react/browser.tsx +++ b/packages/remix-react/browser.tsx @@ -55,7 +55,7 @@ export function RemixBrowser(_props: RemixBrowserProps): ReactElement { // We need to include a wrapper RemixErrorBoundary here in case the root error // boundary also throws and we need to bubble up outside of the router entirely. - // Then we need a stateful location here so the user can back0-button navigate + // Then we need a stateful location here so the user can back-button navigate // out of there let location: Location = useSyncExternalStore( router.subscribe, From f789bbd10763b3d21eb3b6dc3c4af9464ca00d75 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 6 Jan 2023 10:27:04 -0500 Subject: [PATCH 6/6] Add changeset --- .changeset/eighty-hounds-mix.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/eighty-hounds-mix.md diff --git a/.changeset/eighty-hounds-mix.md b/.changeset/eighty-hounds-mix.md new file mode 100644 index 00000000000..e26ca059d4b --- /dev/null +++ b/.changeset/eighty-hounds-mix.md @@ -0,0 +1,5 @@ +--- +"@remix-run/react": patch +--- + +Add outer ErrorBoundary to catch root ErrorBoundary thrown errors