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

Avoid hydration loops when layout error renders also throw #9566

Merged
merged 5 commits into from
Jun 14, 2024
Merged

Avoid hydration loops when layout error renders also throw #9566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aacda63
Avoid hydration loops when layout error renders also throw
brophdawg11 Jun 5, 2024
806cef4
comment clarification
brophdawg11 Jun 5, 2024
cecee37
Add changeset
brophdawg11 Jun 13, 2024
bf1c558
Update approach
brophdawg11 Jun 14, 2024
e228701
Update
brophdawg11 Jun 14, 2024
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
5 changes: 5 additions & 0 deletions .changeset/mighty-experts-agree.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/react": patch
---

Avoid hydration loops when `Layout` `ErrorBoundary` renders also throw
251 changes: 251 additions & 0 deletions integration/root-route-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,4 +153,255 @@ test.describe("root route", () => {

console.error = oldConsoleError;
});

test("Skip the Layout on subsequent server renders if Layout/ErrorBoundary throws (sync)", async ({
page,
}) => {
let oldConsoleError;
oldConsoleError = console.error;
console.error = () => {};

fixture = await createFixture(
{
files: {
"app/root.tsx": js`
import * as React from "react";
import { defer } from "@remix-run/node";
import { Await, Scripts, useRouteError, useRouteLoaderData } from "@remix-run/react";

export function Layout({ children }) {
let data = useRouteLoaderData("root");
return (
<html>
<head>
<title>Layout Title</title>
</head>
<body id="layout">
<p>{data.this.should.throw}</p>
{children}
<Scripts />
</body>
</html>
);
}
export function loader() {
return { ok: true };
}
export default function Root() {
return <p>success</p>;
}
export function ErrorBoundary() {
return <p>error</p>;
}
`,
},
},
ServerMode.Development
);
appFixture = await createAppFixture(fixture, ServerMode.Development);
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/");
// The server should send back the fallback 500 HTML since it was unable
// to render the Layout/ErrorBoundary combo
expect(await app.page.$("#layout")).toBeNull();
expect(await app.getHtml("pre")).toMatch("Unexpected Server Error");
expect(await app.getHtml("pre")).toMatch(
"Cannot read properties of undefined"
);

console.error = oldConsoleError;
});

test("Skip the Layout on subsequent client renders if Layout/ErrorBoundary throws (async)", async ({
page,
}) => {
let oldConsoleError;
oldConsoleError = console.error;
console.error = () => {};

fixture = await createFixture(
{
files: {
"app/root.tsx": js`
import * as React from "react";
import { defer } from "@remix-run/node";
import { Await, Scripts, useRouteError, useRouteLoaderData } from "@remix-run/react";

export function Layout({ children }) {
let data = useRouteLoaderData("root");
return (
<html>
<head>
<title>Layout Title</title>
</head>
<body id="layout">
<React.Suspense fallback={<p id="loading">Loading...</p>}>
<Await resolve={data.lazy}>
{(v) => <p>{v.this.should.throw}</p>}
</Await>
</React.Suspense>
{children}
<Scripts />
</body>
</html>
);
}
export function loader() {
return defer({
// this lets the app hydrate properly, then reject the deferred promise,
// which should throw on the initial render _and_ the error render,
// resulting in us bubbling to the default error boundary and skipping
// our Layout component entirely to avoid a loop
lazy: new Promise((r) => setTimeout(() => r(null), 100)),
});
}
export default function Root() {
return <p>success</p>;
}
export function ErrorBoundary() {
return <p>error</p>;
}
`,
},
},
ServerMode.Development
);
appFixture = await createAppFixture(fixture, ServerMode.Development);
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/", false);
expect(await app.page.$("#layout")).toBeDefined();
expect(await app.getHtml("#loading")).toMatch("Loading...");
await page.waitForSelector("h1");
expect(await app.page.$("#layout")).toBeNull();
expect(await app.getHtml("title")).toMatch("Application Error");
expect(await app.getHtml("h1")).toMatch("Application Error");
expect(await app.getHtml("pre")).toMatch(
"TypeError: Cannot read properties of null"
);

console.error = oldConsoleError;
});

test("Skip the Layout on subsequent server renders if the Layout/DefaultErrorBoundary throws (sync)", async ({
page,
}) => {
let oldConsoleError;
oldConsoleError = console.error;
console.error = () => {};

fixture = await createFixture(
{
files: {
"app/root.tsx": js`
import * as React from "react";
import { defer } from "@remix-run/node";
import { Await, Scripts, useRouteError, useRouteLoaderData } from "@remix-run/react";

export function Layout({ children }) {
let data = useRouteLoaderData("root");
return (
<html>
<head>
<title>Layout Title</title>
</head>
<body id="layout">
<p>{data.this.should.throw}</p>
{children}
<Scripts />
</body>
</html>
);
}
export function loader() {
return { ok: true };
}
export default function Root() {
return <p>success</p>;
}
`,
},
},
ServerMode.Development
);
appFixture = await createAppFixture(fixture, ServerMode.Development);
let app = new PlaywrightFixture(appFixture, page);

await app.goto("/");
// The server should send back the fallback 500 HTML since it was unable
// to render the Layout/ErrorBoundary combo
expect(await app.page.$("#layout")).toBeNull();
expect(await app.getHtml("pre")).toMatch("Unexpected Server Error");
expect(await app.getHtml("pre")).toMatch(
"Cannot read properties of undefined"
);

console.error = oldConsoleError;
});

test("Skip the Layout on subsequent client renders if the Layout/DefaultErrorBoundary throws (async)", async ({
page,
}) => {
let oldConsoleError;
oldConsoleError = console.error;
console.error = () => {};

fixture = await createFixture(
{
files: {
"app/root.tsx": js`
import * as React from "react";
import { defer } from "@remix-run/node";
import { Await, Scripts, useRouteError, useRouteLoaderData } from "@remix-run/react";

export function Layout({ children }) {
let data = useRouteLoaderData("root");
return (
<html>
<head>
<title>Layout Title</title>
</head>
<body id="layout">
<React.Suspense fallback={<p id="loading">Loading...</p>}>
<Await resolve={data.lazy}>
{(v) => <p>{v.this.should.throw}</p>}
</Await>
</React.Suspense>
{children}
<Scripts />
</body>
</html>
);
}
export function loader() {
return defer({
// this lets the app hydrate properly, then reject the deferred promise,
// which should throw on the initial render _and_ the error render,
// resulting in us bubbling to the default error boundary and skipping
// our Layout component entirely to avoid a loop
lazy: new Promise((r) => setTimeout(() => r(null), 100)),
});
}
export default function Root() {
return <p>success</p>;
}
`,
},
},
ServerMode.Development
);
appFixture = await createAppFixture(fixture, ServerMode.Development);
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/", false);
expect(await app.page.$("#layout")).toBeDefined();
expect(await app.getHtml("#loading")).toMatch("Loading...");
await page.waitForSelector("h1");
expect(await app.page.$("#layout")).toBeNull();
expect(await app.getHtml("title")).toMatch("Application Error");
expect(await app.getHtml("h1")).toMatch("Application Error");
expect(await app.getHtml("pre")).toMatch(
"TypeError: Cannot read properties of null"
);

console.error = oldConsoleError;
});
});
41 changes: 37 additions & 4 deletions packages/remix-react/errorBoundaries.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { Scripts, useRemixContext } from "./components";

type RemixErrorBoundaryProps = React.PropsWithChildren<{
location: Location;
isOutsideRemixApp?: boolean;
error?: Error;
}>;

Expand Down Expand Up @@ -53,7 +54,12 @@ export class RemixErrorBoundary extends React.Component<

render() {
if (this.state.error) {
return <RemixRootDefaultErrorBoundary error={this.state.error} />;
return (
<RemixRootDefaultErrorBoundary
error={this.state.error}
isOutsideRemixApp={true}
/>
);
} else {
return this.props.children;
}
Expand All @@ -63,7 +69,13 @@ 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: unknown }) {
export function RemixRootDefaultErrorBoundary({
error,
isOutsideRemixApp,
}: {
error: unknown;
isOutsideRemixApp?: boolean;
}) {
console.error(error);

let heyDeveloper = (
Expand Down Expand Up @@ -103,7 +115,10 @@ export function RemixRootDefaultErrorBoundary({ error }: { error: unknown }) {
}

return (
<BoundaryShell title="Application Error!">
<BoundaryShell
title="Application Error!"
isOutsideRemixApp={isOutsideRemixApp}
>
<h1 style={{ fontSize: "24px" }}>Application Error</h1>
<pre
style={{
Expand All @@ -123,15 +138,33 @@ export function RemixRootDefaultErrorBoundary({ error }: { error: unknown }) {
export function BoundaryShell({
title,
renderScripts,
isOutsideRemixApp,
children,
}: {
title: string;
renderScripts?: boolean;
isOutsideRemixApp?: boolean;
children: React.ReactNode | React.ReactNode[];
}) {
let { routeModules } = useRemixContext();

if (routeModules.root?.Layout) {
// Generally speaking, when the root route has a Layout we want to use that
// as the app shell instead of the default `BoundaryShell` wrapper markup below.
// This is true for `loader`/`action` errors, most render errors, and
// `HydrateFallback` scenarios.

// However, render errors thrown from the `Layout` present a bit of an issue
// because if the `Layout` itself throws during the `ErrorBoundary` pass and
// we bubble outside the `RouterProvider` to the wrapping `RemixErrorBoundary`,
// by returning only `children` here we'll be trying to append a `<div>` to
// the `document` and the DOM will throw, putting React into an error/hydration
// loop.

// Instead, if we're ever rendering from the outermost `RemixErrorBoundary`
// during hydration that wraps `RouterProvider`, then we can't trust the
// `Layout` and should fallback to the default app shell so we're always
// returning an `<html>` document.
if (routeModules.root?.Layout && !isOutsideRemixApp) {
return children;
}

Expand Down