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

Add future.v2_errorBoundary flag #4918

Merged
merged 13 commits into from
Jan 13, 2023
28 changes: 28 additions & 0 deletions .changeset/odd-numbers-film.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
---
"@remix-run/react": patch
"@remix-run/server-runtime": patch
---

Add `future.v2_errorBoundary` flag to opt-into v2 `ErrorBoundary` behavior. This removes the separate `CatchBoundary` and `ErrorBoundary` and consolidates them into a single `ErrorBoundary` following the logic used by `errorElement` in React You can then use `isRouteErrorResponse` to differentiate between thrown `Response`/`Error` instances.
brophdawg11 marked this conversation as resolved.
Show resolved Hide resolved

```jsx
// Current (Remix v1 default)
export function CatchBoundary() {
let caught = useCatch();
return <p>{caught.status} {caught.data}</p>;
}

export function ErrorBoundary({ error }) {
return <p>{error.message}</p>;
}


// Using future.v2_errorBoundary
brophdawg11 marked this conversation as resolved.
Show resolved Hide resolved
export function ErrorBoundary() {
let error = useRouteError();

return isRouteErrorResponse(error) ?
<p>{error.status} {error.data}</p> :
<p>{error.message}</p>;
}
```
237 changes: 237 additions & 0 deletions integration/error-boundary-v2-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,237 @@
import type { Page } from "@playwright/test";
import { test, expect } from "@playwright/test";
import { ServerMode } from "@remix-run/server-runtime/mode";

import { createAppFixture, createFixture, js } from "./helpers/create-fixture";
import type { Fixture, AppFixture } from "./helpers/create-fixture";
import { PlaywrightFixture } from "./helpers/playwright-fixture";

test.describe("V2 Singular ErrorBoundary (future.v2_errorBoundary)", () => {
let fixture: Fixture;
let appFixture: AppFixture;
let oldConsoleError: () => void;

test.beforeAll(async () => {
fixture = await createFixture({
future: {
v2_errorBoundary: true,
},
files: {
"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>
);
}
`,

"app/routes/parent.jsx": js`
import {
Link,
Outlet,
isRouteErrorResponse,
useLoaderData,
useRouteError,
} from "@remix-run/react";

export function loader() {
return "PARENT LOADER";
}

export default function Component() {
return (
<div>
<nav>
<ul>
<li><Link to="/parent/child-with-boundary">Link</Link></li>
<li><Link to="/parent/child-with-boundary?type=error">Link</Link></li>
<li><Link to="/parent/child-with-boundary?type=response">Link</Link></li>
<li><Link to="/parent/child-with-boundary?type=render">Link</Link></li>
<li><Link to="/parent/child-without-boundary?type=error">Link</Link></li>
<li><Link to="/parent/child-without-boundary?type=response">Link</Link></li>
<li><Link to="/parent/child-without-boundary?type=render">Link</Link></li>
</ul>
</nav>
<p id="parent-data">{useLoaderData()}</p>
<Outlet />
</div>
)
}

export function ErrorBoundary() {
let error = useRouteError();
return isRouteErrorResponse(error) ?
<p id="parent-error-response">{error.status + ' ' + error.data}</p> :
<p id="parent-error">{error.message}</p>;
}
`,

"app/routes/parent/child-with-boundary.jsx": js`
import {
isRouteErrorResponse,
useLoaderData,
useLocation,
useRouteError,
} from "@remix-run/react";

export function loader({ request }) {
let errorType = new URL(request.url).searchParams.get('type');
if (errorType === 'response') {
throw new Response('Loader Response', { status: 418 });
} else if (errorType === 'error') {
throw new Error('Loader Error');
}
return "CHILD LOADER";
}

export default function Component() {;
let data = useLoaderData();
if (new URLSearchParams(useLocation().search).get('type') === "render") {
throw new Error("Render Error");
}
return <p id="child-data">{data}</p>;
}

export function ErrorBoundary() {
let error = useRouteError();
return isRouteErrorResponse(error) ?
<p id="child-error-response">{error.status + ' ' + error.data}</p> :
<p id="child-error">{error.message}</p>;
}
`,

"app/routes/parent/child-without-boundary.jsx": js`
import { useLoaderData, useLocation } from "@remix-run/react";

export function loader({ request }) {
let errorType = new URL(request.url).searchParams.get('type');
if (errorType === 'response') {
throw new Response('Loader Response', { status: 418 });
} else if (errorType === 'error') {
throw new Error('Loader Error');
}
return "CHILD LOADER";
}

export default function Component() {;
let data = useLoaderData();
if (new URLSearchParams(useLocation().search).get('type') === "render") {
throw new Error("Render Error");
}
return <p id="child-data">{data}</p>;
}
`,
},
});

appFixture = await createAppFixture(fixture, ServerMode.Development);
});

test.afterAll(() => {
appFixture.close();
});

test.beforeEach(({ page }) => {
oldConsoleError = console.error;
console.error = () => {};
});

test.afterEach(() => {
console.error = oldConsoleError;
});

test.describe("without JavaScript", () => {
test.use({ javaScriptEnabled: false });
runBoundaryTests();
});

test.describe("with JavaScript", () => {
test.use({ javaScriptEnabled: true });
runBoundaryTests();
});

function runBoundaryTests() {
// Shorthand util to wait for an element to appear before asserting it
async function waitForAndAssert(
page: Page,
app: PlaywrightFixture,
selector: string,
match: string
) {
await page.waitForSelector(selector);
expect(await app.getHtml(selector)).toMatch(match);
}

test("No errors", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/parent");
await app.clickLink("/parent/child-with-boundary");
await waitForAndAssert(page, app, "#child-data", "CHILD LOADER");
});

test("Throwing a Response to own boundary", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/parent");
await app.clickLink("/parent/child-with-boundary?type=response");
await waitForAndAssert(
page,
app,
"#child-error-response",
"418 Loader Response"
);
});

test("Throwing an Error to own boundary", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/parent");
await app.clickLink("/parent/child-with-boundary?type=error");
await waitForAndAssert(page, app, "#child-error", "Loader Error");
});

test("Throwing a render error to own boundary", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/parent");
await app.clickLink("/parent/child-with-boundary?type=render");
await waitForAndAssert(page, app, "#child-error", "Render Error");
});

test("Throwing a Response to parent boundary", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/parent");
await app.clickLink("/parent/child-without-boundary?type=response");
await waitForAndAssert(
page,
app,
"#parent-error-response",
"418 Loader Response"
);
});

test("Throwing an Error to parent boundary", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/parent");
await app.clickLink("/parent/child-without-boundary?type=error");
await waitForAndAssert(page, app, "#parent-error", "Loader Error");
});

test("Throwing a render error to parent boundary", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/parent");
await app.clickLink("/parent/child-without-boundary?type=render");
await waitForAndAssert(page, app, "#parent-error", "Render Error");
});
}
});
2 changes: 2 additions & 0 deletions packages/remix-dev/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export type ServerModuleFormat = "esm" | "cjs";
export type ServerPlatform = "node" | "neutral";

interface FutureConfig {
v2_errorBoundary: boolean;
v2_meta: boolean;
}

Expand Down Expand Up @@ -481,6 +482,7 @@ export async function readConfig(
}

let future = {
v2_errorBoundary: appConfig.future?.v2_errorBoundary === true,
v2_meta: appConfig.future?.v2_meta === true,
};

Expand Down
23 changes: 14 additions & 9 deletions packages/remix-react/components.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import {
RemixRootDefaultErrorBoundary,
RemixRootDefaultCatchBoundary,
RemixCatchBoundary,
V2_RemixRootDefaultErrorBoundary,
} from "./errorBoundaries";
import invariant from "./invariant";
import {
Expand Down Expand Up @@ -140,7 +141,7 @@ export function RemixRoute({ id }: { id: string }) {
}

export function RemixRouteError({ id }: { id: string }) {
let { routeModules } = useRemixContext();
let { future, routeModules } = useRemixContext();

// This checks prevent cryptic error messages such as: 'Cannot read properties of undefined (reading 'root')'
invariant(
Expand All @@ -152,14 +153,18 @@ export function RemixRouteError({ id }: { id: string }) {
let error = useRouteError();
let { CatchBoundary, ErrorBoundary } = routeModules[id];

// POC for potential v2 error boundary handling
// if (future.v2_errorBoundary) {
// // Provide defaults for the root route if they are not present
// if (id === "root") {
// ErrorBoundary ||= RemixRootDefaultNewErrorBoundary;
// }
// return <ErrorBoundary />
// }
if (future.v2_errorBoundary) {
// Provide defaults for the root route if they are not present
if (id === "root") {
ErrorBoundary ||= V2_RemixRootDefaultErrorBoundary;
}
if (ErrorBoundary) {
// TODO: Unsure if we can satisfy the typings here
// @ts-expect-error
return <ErrorBoundary />;
}
throw error;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we've opted in, just render the ErrorBoundary directly

}

// Provide defaults for the root route if they are not present
if (id === "root") {
Expand Down
1 change: 1 addition & 0 deletions packages/remix-react/entry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export interface EntryContext extends RemixContextObject {
}

export interface FutureConfig {
v2_errorBoundary: boolean;
v2_meta: boolean;
}

Expand Down
27 changes: 26 additions & 1 deletion packages/remix-react/errorBoundaries.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, { useContext } from "react";
import type { ErrorResponse } from "@remix-run/router";
import type { Location } from "react-router-dom";
import { isRouteErrorResponse, useRouteError } from "react-router-dom";

import type { CatchBoundaryComponent } from "./routeModules";
import type { ThrownResponse } from "./errors";
Expand Down Expand Up @@ -48,6 +48,23 @@ export function RemixRootDefaultErrorBoundary({ error }: { error: Error }) {
);
}

export function V2_RemixRootDefaultErrorBoundary() {
let error = useRouteError();
if (isRouteErrorResponse(error)) {
return <RemixRootDefaultCatchBoundaryImpl caught={error} />;
} else if (error instanceof Error) {
return <RemixRootDefaultErrorBoundary error={error} />;
} else {
let errorString =
error == null
? "Unknown Error"
: typeof error === "object" && "toString" in error
? error.toString()
: JSON.stringify(error);
Comment on lines +119 to +124
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this worth it, maybe we just console.error and fall back to new Error("Unknown Error")?

return <RemixRootDefaultErrorBoundary error={new Error(errorString)} />;
}
}

let RemixCatchContext = React.createContext<ThrownResponse | undefined>(
undefined
);
Expand Down Expand Up @@ -89,6 +106,14 @@ export function RemixCatchBoundary({
*/
export function RemixRootDefaultCatchBoundary() {
let caught = useCatch();
return <RemixRootDefaultCatchBoundaryImpl caught={caught} />;
}

function RemixRootDefaultCatchBoundaryImpl({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted internals into a component that can be used by the default Remix v2 Root Error Boundary

caught,
}: {
caught: ThrownResponse;
}) {
return (
<html lang="en">
<head>
Expand Down
2 changes: 2 additions & 0 deletions packages/remix-react/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export type {
export {
Form,
Outlet,
isRouteErrorResponse,
useBeforeUnload,
useFormAction,
useHref,
Expand All @@ -24,6 +25,7 @@ export {
useParams,
useResolvedPath,
useRevalidator,
useRouteError,
useSearchParams,
useSubmit,
} from "react-router-dom";
Expand Down
Loading