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

Open
wants to merge 6 commits into
base: canary
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 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
@@ -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,11 @@
'use client'

import dynamic from 'next/dynamic'

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

export default function Page() {
return <BrowserOnly />
}
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,9 @@
import Foo from '../foo'

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

import Foo from '../foo'

export default function Page() {
return (
<div>
<Foo />
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* @type {import('next').NextConfig}
*/
const nextConfig = {
experimental: {
reactOwnerStack: true,
},
}

module.exports = nextConfig
Copy link
Member

Choose a reason for hiding this comment

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

Where are the assertions on the owner stack in this test?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't have stacks from user land such as Page component, others are all nextjs frame stack.

Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
import { nextTestSetup } from 'e2e-utils'
import { assertHasRedbox, getRedboxSource } from 'next-test-utils'

async function getStackFramesContent(browser) {
const stackFrameElements = await browser.elementsByCss(
'[data-nextjs-call-stack-frame]'
)
const stackFramesContent = (
await Promise.all(
stackFrameElements.map(async (frame) => {
const functionNameEl = await frame.$('[data-nextjs-frame-expanded]')
const sourceEl = await frame.$('[data-has-source]')
const functionName = functionNameEl
? await functionNameEl.innerText()
: ''
const source = sourceEl ? await sourceEl.innerText() : ''

if (!functionName) {
return ''
}
return `at ${functionName} (${source})`
})
)
)
.filter(Boolean)
.join('\n')

return stackFramesContent
}

describe('app-dir - owner-stack-invalid-element-type', () => {
const { next } = nextTestSetup({
files: __dirname,
})

it('should catch invalid element from on client-only component', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('should catch invalid element from on client-only component', async () => {
it('should catch invalid element from a client-only component', async () => {

I don't see a client-only import in this file. I assume this is just referring to use client?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's using the next/dynamic, ssr: false, testing it's can also cover not only SSR'd client components but also browser only components

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(
`"at Page (app/browser/page.js (10:10))"`
Copy link
Member

Choose a reason for hiding this comment

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

Why is the Component name here different between Webpack and Turbopack? Turbopack has at Page, Webpack has at BrowserOnly

)
expect(source).toMatchInlineSnapshot(`
"app/browser/browser-only.js (8:7) @ BrowserOnly
Copy link
Member

Choose a reason for hiding this comment

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

Why is the Component name here different between Turbopack and Webpack? Turbopack has @ BrowserOnly, Webpack @ Foo

Copy link
Member Author

Choose a reason for hiding this comment

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

It's returned from the source map, retrieved by the line number and column number. There might be some inconsistence between two that we need to tackle later with sourcemap.


6 | return (
7 | <div>
> 8 | <Foo />
| ^
9 | </div>
10 | )
11 | }"
`)
} else {
expect(stackFramesContent).toMatchInlineSnapshot(
`"at BrowserOnly (app/browser/page.js (10:11))"`
)
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 on rsc component', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('should catch invalid element from on rsc component', async () => {
it('should catch invalid element from an 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 (6:7) @ Page

4 | return (
5 | <div>
> 6 | <Foo />
| ^
7 | </div>
8 | )
9 | }"
`)
} else {
expect(stackFramesContent).toMatchInlineSnapshot(`""`)
expect(source).toMatchInlineSnapshot(`
"app/rsc/page.js (6:8) @ Foo

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

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 (8:7) @ Page

6 | return (
7 | <div>
> 8 | <Foo />
| ^
9 | </div>
10 | )
11 | }"
`)
} else {
expect(stackFramesContent).toMatchInlineSnapshot(`""`)
expect(source).toMatchInlineSnapshot(`
"app/ssr/page.js (8:8) @ Foo

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