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.

Sign up for GitHub

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

Add outer RemixErrorBoundary to catch root boundary-thrown errors #5012

Merged
merged 6 commits into from
Jan 6, 2023
Merged

Add outer RemixErrorBoundary to catch root boundary-thrown errors #5012

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
47ad139
Add outer RemixErrorBoundary to catch root boundary-thrown errors
brophdawg11 Jan 5, 2023
026df02
Handle firefox not including error message in stack
brophdawg11 Jan 5, 2023
3d4c163
add types for useSyncExternalStore
brophdawg11 Jan 5, 2023
d2f35cf
Remove duplicate error log
brophdawg11 Jan 5, 2023
edc141d
fix typo
brophdawg11 Jan 6, 2023
f789bbd
Add changeset
brophdawg11 Jan 6, 2023
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
326 changes: 326 additions & 0 deletions integration/error-boundary-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -875,3 +875,329 @@ test.describe("loaderData in ErrorBoundary", () => {
});
}
});

test.describe("Default ErrorBoundary", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document request + SPA transition tests for:

  • no root boundary
  • root boundary
  • root boundary that throws

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 (
<html>
<head />
<body>
<main>
<div>Root Error Boundary</div>
<p id="root-error-boundary">{error.message}</p>
<p>{oh.no.what.have.i.done}</p>
</main>
<Scripts />
</body>
</html>
)
}
`
: js`
export function ErrorBoundary({ error }) {
return (
<html>
<head />
<body>
<main>
<div>Root Error Boundary</div>
<p id="root-error-boundary">{error.message}</p>
</main>
<Scripts />
</body>
</html>
)
}
`;

return {
"app/root.jsx": js`
import { Links, Meta, Outlet, Scripts } from "@remix-run/react";

export default function Root() {
return (
<html lang="en">
<head>
<Meta />
<Links />
</head>
<body>
<main>
<Outlet />
</main>
<Scripts />
</body>
</html>
);
}

${errorBoundaryCode}
`,

"app/routes/index.jsx": js`
import { Link } from "@remix-run/react";
export default function () {
return (
<div>
<h1 id="index">Index</h1>
<Link to="/loader-error">Loader Error</Link>
<Link to="/render-error">Render Error</Link>
</div>
);
}
`,

"app/routes/loader-error.jsx": js`
export function loader() {
throw new Error('Loader Error');
}
export default function () {
return <h1 id="loader-error">Loader Error</h1>
}
`,

"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,
}, 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");
// 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
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,
}, 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");
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");

// 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,
}, 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");
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");

// Ensure we can click back to our prior page
await app.goBack();
await page.waitForSelector("h1#index");
});
});
});
});
23 changes: 22 additions & 1 deletion packages/remix-react/browser.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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
brophdawg11 marked this conversation as resolved.
Show resolved Hide resolved
// out of there
let location: Location = useSyncExternalStore(
router.subscribe,
() => router.state.location,
() => router.state.location
);

return (
<RemixContext.Provider
value={{
Expand All @@ -55,7 +71,12 @@ export function RemixBrowser(_props: RemixBrowserProps): ReactElement {
future: window.__remixContext.future,
}}
>
<RouterProvider router={router} fallbackElement={null} />
<RemixErrorBoundary
location={location}
component={RemixRootDefaultErrorBoundary}
>
<RouterProvider router={router} fallbackElement={null} />
</RemixErrorBoundary>
Comment on lines -58 to +79
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap everything in a last-resort error boundary, and send the router location in so users can back-button out of it

</RemixContext.Provider>
);
}
3 changes: 2 additions & 1 deletion packages/remix-react/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Loading