fix: prevent router errors from being logged on the client (#71583)

Our patched `console.error` tries to skip internal router errors by checking `isNextRouterError(error)`. however, if one of those happened on the server and got replayed on the client, it gets logged differently -- it's prefixed with the `[ Server ]` badge. this changes the position where the actual error object is in `args` and thus messes up our detection, so we end up printing it out (instead of hiding it like we should) This PR adds a check that attempts to match replayed server errors (following [similar logic from react devtools](https://github.com/facebook/react/blob/65a56d0e99261481c721334a3ec4561d173594cd/packages/react-devtools-shared/src/backend/flight/renderer.js#L88-L93)) and thus also filter out router errors that originated on the server Closes #71555
vercel · Oct 22, 2024 · 42090c5 · 42090c5
1 parent 4f260d6
commit 42090c5
Show file tree

Hide file tree

Showing 9 changed files with 195 additions and 4 deletions.
diff --git a/packages/next/src/client/components/globals/intercept-console-error.ts b/packages/next/src/client/components/globals/intercept-console-error.ts
@@ -1,3 +1,4 @@
+import isError from '../../../lib/is-error'
 import { isNextRouterError } from '../is-next-router-error'
 import { handleClientError } from '../react-dev-overlay/internal/helpers/use-error-handler'
 
@@ -11,15 +12,67 @@ export function patchConsoleError() {
   }
 
   window.console.error = (...args: any[]) => {
-    // See https://github.com/facebook/react/blob/d50323eb845c5fde0d720cae888bf35dedd05506/packages/react-reconciler/src/ReactFiberErrorLogger.js#L78
-    const error = process.env.NODE_ENV !== 'production' ? args[1] : args[0]
+    let maybeError: unknown
+    let isReplayed: boolean = false
 
-    if (!isNextRouterError(error)) {
+    if (process.env.NODE_ENV !== 'production') {
+      const replayedError = matchReplayedError(...args)
+      if (replayedError) {
+        maybeError = replayedError
+        isReplayed = true
+      } else {
+        // See https://github.com/facebook/react/blob/d50323eb845c5fde0d720cae888bf35dedd05506/packages/react-reconciler/src/ReactFiberErrorLogger.js#L78
+        maybeError = args[1]
+      }
+    } else {
+      maybeError = args[0]
+    }
+
+    if (!isNextRouterError(maybeError)) {
       if (process.env.NODE_ENV !== 'production') {
-        handleClientError(error, args)
+        handleClientError(
+          // replayed errors have their own complex format string that should be used,
+          // but if we pass the error directly, `handleClientError` will ignore it
+          //
+          // TODO: not passing an error here will make `handleClientError`
+          // create a new Error, so we'll lose the stack. we should make it smarter
+          isReplayed ? undefined : maybeError,
+          args
+        )
       }
 
       originConsoleError.apply(window.console, args)
     }
   }
 }
+
+function matchReplayedError(...args: unknown[]): Error | null {
+  // See
+  // https://github.com/facebook/react/blob/65a56d0e99261481c721334a3ec4561d173594cd/packages/react-devtools-shared/src/backend/flight/renderer.js#L88-L93
+  //
+  // Logs replayed from the server look like this:
+  // [
+  //   "%c%s%c %o\n\n%s\n\n%s\n",
+  //   "background: #e6e6e6; ...",
+  //   " Server ", // can also be e.g. " Prerender "
+  //   "",
+  //   Error
+  //   "The above error occurred in the <Page> component."
+  //   ...
+  // ]
+  if (
+    args.length > 3 &&
+    typeof args[0] === 'string' &&
+    args[0].startsWith('%c%s%c ') &&
+    typeof args[1] === 'string' &&
+    typeof args[2] === 'string' &&
+    typeof args[3] === 'string'
+  ) {
+    const maybeError = args[4]
+    if (isError(maybeError)) {
+      return maybeError
+    }
+  }
+
+  return null
+}
diff --git a/test/development/replayed-internal-errors/app/layout.tsx b/test/development/replayed-internal-errors/app/layout.tsx
@@ -0,0 +1,10 @@
+import * as React from 'react'
+
+export default function RootLayout({ children }) {
+  return (
+    <html>
+      <head />
+      <body>{children}</body>
+    </html>
+  )
+}
diff --git a/test/development/replayed-internal-errors/app/not-found.tsx b/test/development/replayed-internal-errors/app/not-found.tsx
@@ -0,0 +1,11 @@
+import * as React from 'react'
+import Link from 'next/link'
+
+export default function Page() {
+  return (
+    <>
+      <h1>Not found</h1>
+      <Link href="/">go back</Link>
+    </>
+  )
+}
diff --git a/test/development/replayed-internal-errors/app/page.tsx b/test/development/replayed-internal-errors/app/page.tsx
@@ -0,0 +1,12 @@
+import * as React from 'react'
+import Link from 'next/link'
+
+/** Add your relevant code here for the issue to reproduce */
+export default function Home() {
+  return (
+    <>
+      <Link href="/will-redirect">Go to a page that calls redirect()</Link>
+      <Link href="/will-notfound">Go to a page that calls notFound()</Link>
+    </>
+  )
+}
diff --git a/test/development/replayed-internal-errors/app/redirect-target/page.tsx b/test/development/replayed-internal-errors/app/redirect-target/page.tsx
@@ -0,0 +1,11 @@
+import * as React from 'react'
+import Link from 'next/link'
+
+export default function Page() {
+  return (
+    <>
+      <h1>Redirected</h1>
+      <Link href="/">go back</Link>
+    </>
+  )
+}
diff --git a/test/development/replayed-internal-errors/app/will-notfound/page.tsx b/test/development/replayed-internal-errors/app/will-notfound/page.tsx
@@ -0,0 +1,6 @@
+import { notFound } from 'next/navigation'
+
+export default function Page() {
+  console.error(new Error('This error should get replayed'))
+  notFound() // ...and this one shouldn't
+}
diff --git a/test/development/replayed-internal-errors/app/will-redirect/page.tsx b/test/development/replayed-internal-errors/app/will-redirect/page.tsx
@@ -0,0 +1,6 @@
+import { redirect } from 'next/navigation'
+
+export default function Page() {
+  console.error(new Error('This error should get replayed'))
+  redirect('/redirect-target') // ...and this one shouldn't
+}
diff --git a/test/development/replayed-internal-errors/index.test.ts b/test/development/replayed-internal-errors/index.test.ts
@@ -0,0 +1,77 @@
+/* eslint-env jest */
+import { nextTestSetup } from 'e2e-utils'
+import { retry } from '../../lib/next-test-utils'
+
+describe('Replaying internal errors', () => {
+  const { next } = nextTestSetup({ files: __dirname })
+
+  it('should not log the internal error thrown by redirect()', async () => {
+    const EXPECTED_REPLAYED_MESSAGE = 'This error should get replayed'
+    const OMITTED_ERROR_MESSAGE = 'NEXT_REDIRECT'
+
+    const browser = await next.browser('/')
+
+    await browser.elementByCss('a[href="/will-redirect"]').click()
+    await retry(async () => {
+      expect(await browser.elementByCss('h1').text()).toBe('Redirected')
+    })
+
+    expect(next.cliOutput).toContain(EXPECTED_REPLAYED_MESSAGE)
+    expect(next.cliOutput).not.toContain(OMITTED_ERROR_MESSAGE)
+
+    // It'd be good to check for redbox here,
+    // but it seems to disappear the first time we navigate to /target.
+    // But checking console errors should be enough because they're closely tied
+
+    const logs = await browser.log()
+
+    expect(logs).toContainEqual(
+      expect.objectContaining({
+        source: 'error',
+        message: expect.stringContaining(EXPECTED_REPLAYED_MESSAGE),
+      })
+    )
+
+    expect(logs).not.toContainEqual(
+      expect.objectContaining({
+        source: 'error',
+        message: expect.stringContaining(OMITTED_ERROR_MESSAGE),
+      })
+    )
+  })
+
+  it('should not log the internal error thrown by notFound()', async () => {
+    const EXPECTED_REPLAYED_MESSAGE = 'This error should get replayed'
+    const OMITTED_ERROR_MESSAGE = 'NEXT_NOT_FOUND'
+
+    const browser = await next.browser('/')
+
+    await browser.elementByCss('a[href="/will-notfound"]').click()
+    await retry(async () => {
+      expect(await browser.elementByCss('h1').text()).toBe('Not found')
+    })
+
+    expect(next.cliOutput).toContain(EXPECTED_REPLAYED_MESSAGE)
+    expect(next.cliOutput).not.toContain(OMITTED_ERROR_MESSAGE)
+
+    // It'd be good to check for redbox here,
+    // but it seems to disappear the first time we navigate to /target.
+    // But checking console errors should be enough because they're closely tied
+
+    const logs = await browser.log()
+
+    expect(logs).toContainEqual(
+      expect.objectContaining({
+        source: 'error',
+        message: expect.stringContaining(EXPECTED_REPLAYED_MESSAGE),
+      })
+    )
+
+    expect(logs).not.toContainEqual(
+      expect.objectContaining({
+        source: 'error',
+        message: expect.stringContaining(OMITTED_ERROR_MESSAGE),
+      })
+    )
+  })
+})
diff --git a/test/development/replayed-internal-errors/next.config.mjs b/test/development/replayed-internal-errors/next.config.mjs
@@ -0,0 +1,5 @@
+/**
+ * @type {import('next').NextConfig}
+ */
+const nextConfig = {}
+export default nextConfig