From 42090c596ee239e41629fec02ca90dea77c80d5e Mon Sep 17 00:00:00 2001 From: Janka Uryga Date: Tue, 22 Oct 2024 15:34:37 +0200 Subject: [PATCH] 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 --- .../globals/intercept-console-error.ts | 61 ++++++++++++++- .../replayed-internal-errors/app/layout.tsx | 10 +++ .../app/not-found.tsx | 11 +++ .../replayed-internal-errors/app/page.tsx | 12 +++ .../app/redirect-target/page.tsx | 11 +++ .../app/will-notfound/page.tsx | 6 ++ .../app/will-redirect/page.tsx | 6 ++ .../replayed-internal-errors/index.test.ts | 77 +++++++++++++++++++ .../replayed-internal-errors/next.config.mjs | 5 ++ 9 files changed, 195 insertions(+), 4 deletions(-) create mode 100644 test/development/replayed-internal-errors/app/layout.tsx create mode 100644 test/development/replayed-internal-errors/app/not-found.tsx create mode 100644 test/development/replayed-internal-errors/app/page.tsx create mode 100644 test/development/replayed-internal-errors/app/redirect-target/page.tsx create mode 100644 test/development/replayed-internal-errors/app/will-notfound/page.tsx create mode 100644 test/development/replayed-internal-errors/app/will-redirect/page.tsx create mode 100644 test/development/replayed-internal-errors/index.test.ts create mode 100644 test/development/replayed-internal-errors/next.config.mjs diff --git a/packages/next/src/client/components/globals/intercept-console-error.ts b/packages/next/src/client/components/globals/intercept-console-error.ts index 056f864f859c1..81aaf9fec029c 100644 --- 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 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 new file mode 100644 index 0000000000000..e7ddfd80c47cd --- /dev/null +++ b/test/development/replayed-internal-errors/app/layout.tsx @@ -0,0 +1,10 @@ +import * as React from 'react' + +export default function RootLayout({ children }) { + return ( + + + {children} + + ) +} diff --git a/test/development/replayed-internal-errors/app/not-found.tsx b/test/development/replayed-internal-errors/app/not-found.tsx new file mode 100644 index 0000000000000..c139c80e6701b --- /dev/null +++ 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 ( + <> +

Not found

+ go back + + ) +} diff --git a/test/development/replayed-internal-errors/app/page.tsx b/test/development/replayed-internal-errors/app/page.tsx new file mode 100644 index 0000000000000..2ea2e805f444e --- /dev/null +++ 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 ( + <> + Go to a page that calls redirect() + Go to a page that calls notFound() + + ) +} diff --git a/test/development/replayed-internal-errors/app/redirect-target/page.tsx b/test/development/replayed-internal-errors/app/redirect-target/page.tsx new file mode 100644 index 0000000000000..38643d18a41ec --- /dev/null +++ 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 ( + <> +

Redirected

+ go back + + ) +} diff --git a/test/development/replayed-internal-errors/app/will-notfound/page.tsx b/test/development/replayed-internal-errors/app/will-notfound/page.tsx new file mode 100644 index 0000000000000..63c1ee9b6962f --- /dev/null +++ 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 new file mode 100644 index 0000000000000..f88643822da22 --- /dev/null +++ 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 new file mode 100644 index 0000000000000..23908fe7c00f2 --- /dev/null +++ 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 new file mode 100644 index 0000000000000..b7f46391c586b --- /dev/null +++ b/test/development/replayed-internal-errors/next.config.mjs @@ -0,0 +1,5 @@ +/** + * @type {import('next').NextConfig} + */ +const nextConfig = {} +export default nextConfig