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

Display the stitched error instead of react error #72106

Merged
merged 14 commits into from
Nov 5, 2024
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import React from 'react'
import { ACTION_UNHANDLED_ERROR, type OverlayState } from '../shared'
import type { OverlayState } from '../shared'
import { ShadowPortal } from '../internal/components/ShadowPortal'
import { BuildError } from '../internal/container/BuildError'
import { Errors, type SupportedErrorEvent } from '../internal/container/Errors'
import { parseStack } from '../internal/helpers/parse-stack'
import { Errors } from '../internal/container/Errors'
import { StaticIndicator } from '../internal/container/StaticIndicator'
import { Base } from '../internal/styles/Base'
import { ComponentStyles } from '../internal/styles/ComponentStyles'
Expand All @@ -13,7 +12,7 @@ import type { Dispatcher } from './hot-reloader-client'
import { RuntimeErrorHandler } from '../internal/helpers/runtime-error-handler'

interface ReactDevOverlayState {
reactError: SupportedErrorEvent | null
isReactError: boolean
}
export default class ReactDevOverlay extends React.PureComponent<
{
Expand All @@ -23,27 +22,20 @@ export default class ReactDevOverlay extends React.PureComponent<
},
ReactDevOverlayState
> {
state = { reactError: null }
state = { isReactError: false }

static getDerivedStateFromError(error: Error): ReactDevOverlayState {
if (!error.stack) return { reactError: null }
if (!error.stack) return { isReactError: false }

RuntimeErrorHandler.hadRuntimeError = true
return {
reactError: {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not used anymore, I didn't figure out last time how to get rid of this. Some tests were failing when I replace [reactError] with state.errors.

This time I found that it's the issue with not reporting error.cause in onRecoverableError, otherwise it's erroring with "There was an error during concurrent rendering but React was able to recover by instead synchronously rendering the entire root.". The critical change is in onRecoverableError

id: 0,
event: {
type: ACTION_UNHANDLED_ERROR,
reason: error,
frames: parseStack(error.stack),
},
},
isReactError: true,
}
}

render() {
const { state, children, dispatcher } = this.props
const { reactError } = this.state
const { isReactError } = this.state

const hasBuildError = state.buildError != null
const hasRuntimeErrors = Boolean(state.errors.length)
Expand All @@ -52,7 +44,7 @@ export default class ReactDevOverlay extends React.PureComponent<

return (
<>
{reactError ? (
{isReactError ? (
<html>
<head></head>
<body></body>
Expand All @@ -78,8 +70,10 @@ export default class ReactDevOverlay extends React.PureComponent<
{hasRuntimeErrors ? (
<Errors
isAppDir={true}
initialDisplayState={reactError ? 'fullscreen' : 'minimized'}
errors={reactError ? [reactError] : state.errors}
initialDisplayState={
isReactError ? 'fullscreen' : 'minimized'
}
errors={state.errors}
versionInfo={state.versionInfo}
hasStaticIndicator={hasStaticIndicator}
debugInfo={debugInfo}
Expand Down
9 changes: 6 additions & 3 deletions packages/next/src/client/react-client-callbacks/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,21 @@ import type { HydrationOptions } from 'react-dom/client'
import { isBailoutToCSRError } from '../../shared/lib/lazy-dynamic/bailout-to-csr'
import { reportGlobalError } from './report-global-error'
import { getReactStitchedError } from '../components/react-dev-overlay/internal/helpers/stitched-error'
import isError from '../../lib/is-error'

export const onRecoverableError: HydrationOptions['onRecoverableError'] = (
err,
error,
errorInfo
) => {
const stitchedError = getReactStitchedError(err)
// x-ref: https://github.com/facebook/react/pull/28736
const cause = isError(error) && 'cause' in error ? error.cause : error
const stitchedError = getReactStitchedError(cause)
// In development mode, pass along the component stack to the error
if (process.env.NODE_ENV === 'development' && errorInfo.componentStack) {
;(stitchedError as any)._componentStack = errorInfo.componentStack
}
// Skip certain custom errors which are not expected to be reported on client
if (isBailoutToCSRError(err)) return
if (isBailoutToCSRError(cause)) return

reportGlobalError(stitchedError)
}
37 changes: 18 additions & 19 deletions test/development/acceptance-app/dynamic-error.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,31 +7,30 @@ import { outdent } from 'outdent'
describe('dynamic = "error" in devmode', () => {
const { next } = nextTestSetup({
files: new FileRef(path.join(__dirname, 'fixtures', 'default-template')),
skipStart: true,
})

it('should show error overlay when dynamic is forced', async () => {
const { session, cleanup } = await sandbox(next, undefined, '/server')

// dynamic = "error" and force dynamic
await session.patch(
'app/server/page.js',
outdent`
import { cookies } from 'next/headers';

import Component from '../../index'

export default async function Page() {
await cookies()
return <Component />
}

export const dynamic = "error"
`
const { session, cleanup } = await sandbox(
next,
new Map([
[
'app/server/page.js',
outdent`
import { cookies } from 'next/headers';

export default async function Page() {
await cookies()
return null
}

export const dynamic = "error"
`,
],
]),
'/server'
)

await session.assertHasRedbox()
console.log(await session.getRedboxDescription())
expect(await session.getRedboxDescription()).toMatchInlineSnapshot(
`"[ 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"`
)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
'use client'

import Foo from '../foo'

export default function BrowserOnly() {
return (
<div>
<Foo />
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
'use client'

import dynamic from 'next/dynamic'

const BrowserOnly = dynamic(() => import('./browser-only'), {
ssr: false,
})

// Intermediate component for testing owner stack
function Inner() {
return <BrowserOnly />
}

export default function Page() {
return <Inner />
}
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { ReactNode } from 'react'
export default function Root({ children }: { children: ReactNode }) {
return (
<html>
<body>{children}</body>
</html>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import Foo from '../foo'

// Intermediate component for testing owner stack
function Inner() {
return <Foo />
}

export default function Page() {
return (
<div>
<Inner />
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
'use client'

import Foo from '../foo'

// Intermediate component for testing owner stack
function Inner() {
return <Foo />
}

export default function Page() {
return (
<div>
<Inner />
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
process.env.TEST_OWNER_STACK = 'false'

import { nextTestSetup } from 'e2e-utils'
import {
assertHasRedbox,
getRedboxSource,
getStackFramesContent,
} from 'next-test-utils'

// TODO: When owner stack is enabled by default, remove the condition and only keep one test
const isOwnerStackEnabled =
process.env.TEST_OWNER_STACK !== 'false' ||
process.env.__NEXT_EXPERIMENTAL_PPR === 'true'

;(isOwnerStackEnabled ? describe.skip : describe)(
'app-dir - invalid-element-type',
() => {
const { next } = nextTestSetup({
files: __dirname,
})

it('should catch invalid element from a browser only component', async () => {
const browser = await next.browser('/browser')

await assertHasRedbox(browser)
const source = await getRedboxSource(browser)

const stackFramesContent = await getStackFramesContent(browser)
if (process.env.TURBOPACK) {
expect(stackFramesContent).toMatchInlineSnapshot(`""`)
expect(source).toMatchInlineSnapshot(`
"app/browser/browser-only.js (8:7) @ BrowserOnly

6 | return (
7 | <div>
> 8 | <Foo />
| ^
9 | </div>
10 | )
11 | }"
`)
} else {
expect(stackFramesContent).toMatchInlineSnapshot(`""`)
// FIXME: the methodName should be `@ BrowserOnly` instead of `@ Foo`
expect(source).toMatchInlineSnapshot(`
"app/browser/browser-only.js (8:8) @ Foo

6 | return (
7 | <div>
> 8 | <Foo />
| ^
9 | </div>
10 | )
11 | }"
`)
}
})

it('should catch invalid element from a rsc component', async () => {
const browser = await next.browser('/rsc')

await assertHasRedbox(browser)
const stackFramesContent = await getStackFramesContent(browser)
const source = await getRedboxSource(browser)

if (process.env.TURBOPACK) {
expect(stackFramesContent).toMatchInlineSnapshot(`""`)
expect(source).toMatchInlineSnapshot(`
"app/rsc/page.js (5:10) @ Inner

3 | // Intermediate component for testing owner stack
4 | function Inner() {
> 5 | return <Foo />
| ^
6 | }
7 |
8 | export default function Page() {"
`)
} else {
expect(stackFramesContent).toMatchInlineSnapshot(`""`)
// FIXME: the methodName should be `@ Inner` instead of `@ Foo`
expect(source).toMatchInlineSnapshot(`
"app/rsc/page.js (5:11) @ Foo

3 | // Intermediate component for testing owner stack
4 | function Inner() {
> 5 | return <Foo />
| ^
6 | }
7 |
8 | export default function Page() {"
`)
}
})

it('should catch invalid element from on ssr client component', async () => {
const browser = await next.browser('/ssr')

await assertHasRedbox(browser)

const stackFramesContent = await getStackFramesContent(browser)
const source = await getRedboxSource(browser)
if (process.env.TURBOPACK) {
expect(stackFramesContent).toMatchInlineSnapshot(`""`)
expect(source).toMatchInlineSnapshot(`
"app/ssr/page.js (7:10) @ Inner

5 | // Intermediate component for testing owner stack
6 | function Inner() {
> 7 | return <Foo />
| ^
8 | }
9 |
10 | export default function Page() {"
`)
} else {
expect(stackFramesContent).toMatchInlineSnapshot(`""`)
// FIXME: the methodName should be `@ Inner` instead of `@ Foo`
expect(source).toMatchInlineSnapshot(`
"app/ssr/page.js (7:11) @ Foo

5 | // Intermediate component for testing owner stack
6 | function Inner() {
> 7 | return <Foo />
| ^
8 | }
9 |
10 | export default function Page() {"
`)
}
})
}
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* @type {import('next').NextConfig}
*/
const nextConfig = {
experimental: {
reactOwnerStack: process.env.TEST_OWNER_STACK !== 'false',
},
}

module.exports = nextConfig
Loading
Loading