From c33a3a2c22c7ed49d53a9e08a79bfba874a8ee21 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 14 Jun 2024 10:47:06 -0400 Subject: [PATCH] Avoid hydration loops when layout error renders also throw (#9566) --- .changeset/mighty-experts-agree.md | 5 + integration/root-route-test.ts | 251 +++++++++++++++++++++++ packages/remix-react/errorBoundaries.tsx | 41 +++- 3 files changed, 293 insertions(+), 4 deletions(-) create mode 100644 .changeset/mighty-experts-agree.md diff --git a/.changeset/mighty-experts-agree.md b/.changeset/mighty-experts-agree.md new file mode 100644 index 00000000000..707af186750 --- /dev/null +++ b/.changeset/mighty-experts-agree.md @@ -0,0 +1,5 @@ +--- +"@remix-run/react": patch +--- + +Avoid hydration loops when `Layout` `ErrorBoundary` renders also throw diff --git a/integration/root-route-test.ts b/integration/root-route-test.ts index b72ee98b1e6..903cdedf6a7 100644 --- a/integration/root-route-test.ts +++ b/integration/root-route-test.ts @@ -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 ( + + + Layout Title + + +

{data.this.should.throw}

+ {children} + + + + ); + } + export function loader() { + return { ok: true }; + } + export default function Root() { + return

success

; + } + export function ErrorBoundary() { + return

error

; + } + `, + }, + }, + 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 ( + + + Layout Title + + + Loading...

}> + + {(v) =>

{v.this.should.throw}

} + + + {children} + + + + ); + } + 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

success

; + } + export function ErrorBoundary() { + return

error

; + } + `, + }, + }, + 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 ( + + + Layout Title + + +

{data.this.should.throw}

+ {children} + + + + ); + } + export function loader() { + return { ok: true }; + } + export default function Root() { + return

success

; + } + `, + }, + }, + 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 ( + + + Layout Title + + + Loading...

}> + + {(v) =>

{v.this.should.throw}

} + + + {children} + + + + ); + } + 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

success

; + } + `, + }, + }, + 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; + }); }); diff --git a/packages/remix-react/errorBoundaries.tsx b/packages/remix-react/errorBoundaries.tsx index 5f9caf4d723..2ab1d46ba8b 100644 --- a/packages/remix-react/errorBoundaries.tsx +++ b/packages/remix-react/errorBoundaries.tsx @@ -6,6 +6,7 @@ import { Scripts, useRemixContext } from "./components"; type RemixErrorBoundaryProps = React.PropsWithChildren<{ location: Location; + isOutsideRemixApp?: boolean; error?: Error; }>; @@ -53,7 +54,12 @@ export class RemixErrorBoundary extends React.Component< render() { if (this.state.error) { - return ; + return ( + + ); } else { return this.props.children; } @@ -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 = ( @@ -103,7 +115,10 @@ export function RemixRootDefaultErrorBoundary({ error }: { error: unknown }) { } return ( - +

Application Error

` 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 `` document.
+  if (routeModules.root?.Layout && !isOutsideRemixApp) {
     return children;
   }