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.

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

fix: handle server component replay error in error overlay #71772

Merged
merged 7 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,11 @@ export function patchConsoleError() {

window.console.error = (...args: any[]) => {
let maybeError: unknown
let isReplayed: boolean = false

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]
Expand All @@ -33,10 +31,7 @@ export function patchConsoleError() {
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,
maybeError,
args
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,22 @@ function ErrorDescription({
error: Error
hydrationWarning: string | null
}) {
const isUnhandledError = isUnhandledConsoleOrRejection(error)
// If there's hydration warning or console error, skip displaying the error name
const isUnhandledOrReplayError = isUnhandledConsoleOrRejection(error)
// If the error is:
// - hydration warning
// - captured console error or unhandled rejection
// skip displaying the error name
const title =
isUnhandledOrReplayError || hydrationWarning ? '' : error.name + ': '

// If it's replayed error, display the environment name
const environmentName =
'environmentName' in error ? error['environmentName'] : ''
const envPrefix = environmentName ? `[ ${environmentName} ] ` : ''
return (
<>
{isUnhandledError || hydrationWarning ? '' : error.name + ': '}
{envPrefix}
{title}
<HotlinkedText
text={hydrationWarning || error.message}
matcher={isNextjsLink}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import type { StackFrame } from 'next/dist/compiled/stacktrace-parser'
import type { OriginalStackFrameResponse } from '../../server/shared'
import { isWebpackBundled, formatFrameSourceFile } from './webpack-module-path'
import {
isWebpackInternalResource,
formatFrameSourceFile,
} from './webpack-module-path'
export interface OriginalStackFrame extends OriginalStackFrameResponse {
error: boolean
reason: string | null
Expand Down Expand Up @@ -106,7 +109,7 @@ export function getOriginalStackFrames(
export function getFrameSource(frame: StackFrame): string {
if (!frame.file) return ''

const isWebpackFrame = isWebpackBundled(frame.file)
const isWebpackFrame = isWebpackInternalResource(frame.file)

let str = ''
// Skip URL parsing for webpack internal file paths.
Expand Down Expand Up @@ -137,11 +140,18 @@ export function getFrameSource(frame: StackFrame): string {
}
}

if (!isWebpackBundled(frame.file) && frame.lineNumber != null) {
if (frame.column != null) {
str += ` (${frame.lineNumber}:${frame.column})`
} else {
str += ` (${frame.lineNumber})`
if (!isWebpackInternalResource(frame.file) && frame.lineNumber != null) {
// If the method name is replayed from server, e.g. Page [Server],
// and it's formatted to empty string, recover it as <anonymous> to display it.
if (!str && frame.methodName.endsWith(' [Server]')) {
str = '<anonymous>'
}
if (str) {
if (frame.column != null) {
str += ` (${frame.lineNumber}:${frame.column})`
} else {
str += ` (${frame.lineNumber})`
}
}
}
return str
Expand Down
Original file line number Diff line number Diff line change
@@ -1,32 +1,44 @@
import { formatFrameSourceFile, isWebpackBundled } from './webpack-module-path'
import {
formatFrameSourceFile,
isWebpackInternalResource,
} from './webpack-module-path'

describe('webpack-module-path', () => {
describe('isWebpackBundled', () => {
describe('isWebpackInternalResource', () => {
it('should return true for webpack-internal paths', () => {
expect(isWebpackBundled('webpack-internal:///./src/hello.tsx')).toBe(true)
expect(
isWebpackBundled(
isWebpackInternalResource('webpack-internal:///./src/hello.tsx')
).toBe(true)
expect(
isWebpackInternalResource(
'rsc://React/Server/webpack-internal:///(rsc)/./src/hello.tsx?42'
)
).toBe(true)
expect(
isWebpackBundled(
isWebpackInternalResource(
'rsc://React/Server/webpack:///(rsc)/./src/hello.tsx?42'
)
).toBe(true)
expect(
isWebpackBundled(
isWebpackInternalResource(
'rsc://React/Server/webpack:///(app-pages-browser)/./src/hello.tsx?42'
)
).toBe(true)
expect(isWebpackBundled('webpack://_N_E/./src/hello.tsx')).toBe(true)
expect(isWebpackBundled('webpack://./src/hello.tsx')).toBe(true)
expect(isWebpackBundled('webpack:///./src/hello.tsx')).toBe(true)
expect(
isWebpackInternalResource(
'rsc://React/Server/webpack:///(app-pages-browser)/./src/hello.tsx?42dc'
)
).toBe(true)
expect(isWebpackInternalResource('webpack://_N_E/./src/hello.tsx')).toBe(
true
)
expect(isWebpackInternalResource('webpack://./src/hello.tsx')).toBe(true)
expect(isWebpackInternalResource('webpack:///./src/hello.tsx')).toBe(true)
})

it('should return false for non-webpack-internal paths', () => {
expect(isWebpackBundled('<anonymous>')).toBe(false)
expect(isWebpackBundled('file:///src/hello.tsx')).toBe(false)
expect(isWebpackInternalResource('<anonymous>')).toBe(false)
expect(isWebpackInternalResource('file:///src/hello.tsx')).toBe(false)
})
})

Expand All @@ -50,6 +62,16 @@ describe('webpack-module-path', () => {
'rsc://React/Server/webpack:///(app-pages-browser)/./src/hello.tsx?42'
)
).toBe('./src/hello.tsx')
expect(
formatFrameSourceFile(
'rsc://React/Server/webpack:///(app-pages-browser)/./src/hello.tsx?42?0'
)
).toBe('./src/hello.tsx')
expect(
formatFrameSourceFile(
'rsc://React/Server/webpack:///(app-pages-browser)/./src/hello.tsx?42dc'
)
).toBe('./src/hello.tsx')
expect(formatFrameSourceFile('webpack://_N_E/./src/hello.tsx')).toBe(
'./src/hello.tsx'
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@ const replacementRegExes = [
/^(rsc:\/\/React\/[^/]+\/)/,
/^webpack-internal:\/\/\/(\([\w-]+\)\/)?/,
/^(webpack:\/\/\/|webpack:\/\/(_N_E\/)?)(\([\w-]+\)\/)?/,
/\?\w+(\?\d+)?$/, // React replay error query param, .e.g. ?c69d?0, ?c69d
/\?\d+$/, // React's fakeFunctionIdx query param
]

export function isWebpackBundled(file: string) {
export function isWebpackInternalResource(file: string) {
for (const regex of replacementRegExes) {
if (regex.test(file)) return true

Expand All @@ -19,6 +20,7 @@ export function isWebpackBundled(file: string) {
* Format the webpack internal id to original file path
* webpack-internal:///./src/hello.tsx => ./src/hello.tsx
* rsc://React/Server/webpack-internal:///(rsc)/./src/hello.tsx?42 => ./src/hello.tsx
* rsc://React/Server/webpack:///app/indirection.tsx?14cb?0 => app/indirection.tsx
* webpack://_N_E/./src/hello.tsx => ./src/hello.tsx
* webpack://./src/hello.tsx => ./src/hello.tsx
* webpack:///./src/hello.tsx => ./src/hello.tsx
Expand Down
2 changes: 1 addition & 1 deletion test/development/acceptance-app/dynamic-error.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('dynamic = "error" in devmode', () => {
await session.assertHasRedbox()
console.log(await session.getRedboxDescription())
expect(await session.getRedboxDescription()).toMatchInlineSnapshot(
`"Error: Route /server with \`dynamic = "error"\` couldn't be rendered statically because it used \`cookies\`. See more info here: https://nextjs.org/docs/app/building-your-application/rendering/static-and-dynamic#dynamic-rendering"`
`"[ Server ] Error: Route /server with \`dynamic = "error"\` couldn't be rendered statically because it used \`cookies\`. See more info here: https://nextjs.org/docs/app/building-your-application/rendering/static-and-dynamic#dynamic-rendering"`
)

await cleanup()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,12 +196,12 @@ describe('app-dir - capture-console-error', () => {
{
"callStacks": "",
"count": 1,
"description": "[ Server ] Error: boom",
"source": "app/rsc/page.js (2:11) @ Page
"description": "[ Server ] Error: boom",
"source": "app/rsc/page.js (2:17) @ Page
1 | export default function Page() {
> 2 | console.error(new Error('boom'))
| ^
| ^
3 | return <p>rsc</p>
4 | }
5 |",
Expand All @@ -212,12 +212,12 @@ describe('app-dir - capture-console-error', () => {
{
"callStacks": "",
"count": 1,
"description": "[ Server ] Error: boom",
"source": "app/rsc/page.js (2:11) @ error
"description": "[ Server ] Error: boom",
"source": "app/rsc/page.js (2:17) @ Page
1 | export default function Page() {
> 2 | console.error(new Error('boom'))
| ^
| ^
3 | return <p>rsc</p>
4 | }
5 |",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export default async function Page() {
await new Promise((r) => setTimeout(r, 200))
return <p>Page</p>
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { nextTestSetup } from 'e2e-utils'
import {
assertHasRedbox,
getRedboxCallStack,
getRedboxDescription,
hasErrorToast,
retry,
Expand All @@ -20,7 +22,7 @@ describe('Dynamic IO Dev Errors', () => {
await waitForAndOpenRuntimeError(browser)

expect(await getRedboxDescription(browser)).toMatchInlineSnapshot(
`"[ Server ] Error: Route "/error" used \`Math.random()\` outside of \`"use cache"\` and without explicitly calling \`await connection()\` beforehand. See more info here: https://nextjs.org/docs/messages/next-prerender-random"`
`"[ Server ] Error: Route "/error" used \`Math.random()\` outside of \`"use cache"\` and without explicitly calling \`await connection()\` beforehand. See more info here: https://nextjs.org/docs/messages/next-prerender-random"`
)
})
})
Expand All @@ -38,8 +40,35 @@ describe('Dynamic IO Dev Errors', () => {
await waitForAndOpenRuntimeError(browser)

expect(await getRedboxDescription(browser)).toMatchInlineSnapshot(
`"[ Server ] Error: Route "/error" used \`Math.random()\` outside of \`"use cache"\` and without explicitly calling \`await connection()\` beforehand. See more info here: https://nextjs.org/docs/messages/next-prerender-random"`
`"[ Server ] Error: Route "/error" used \`Math.random()\` outside of \`"use cache"\` and without explicitly calling \`await connection()\` beforehand. See more info here: https://nextjs.org/docs/messages/next-prerender-random"`
)
})
})

it('should display error when component accessed data without suspense boundary', async () => {
const browser = await next.browser('/no-accessed-data')

await retry(async () => {
expect(await hasErrorToast(browser)).toBe(true)
await waitForAndOpenRuntimeError(browser)
await assertHasRedbox(browser)
})

const description = await getRedboxDescription(browser)
const stack = await getRedboxCallStack(browser)
const result = {
description,
stack,
}

expect(result).toMatchInlineSnapshot(`
{
"description": "[ Server ] Error: In Route "/no-accessed-data" this component accessed data without a Suspense boundary above it to provide a fallback UI. See more info: https://nextjs.org/docs/messages/next-prerender-data",
"stack": "Page [Server]
<anonymous> (2:1)
Root [Server]
<anonymous> (2:1)",
}
`)
})
})
Loading