Skip to content

Commit

Permalink
Avoid hydration loops when layout error renders also throw (#9566)
Browse files Browse the repository at this point in the history
  • Loading branch information
@brophdawg11
brophdawg11 authored Jun 14, 2024
1 parent 7a5339b commit c33a3a2
Show file tree
Hide file tree
Showing 3 changed files with 293 additions and 4 deletions.
5 changes: 5 additions & 0 deletions .changeset/mighty-experts-agree.md
View file
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
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
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

0 comments on commit c33a3a2

Please sign in to comment.