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.

Sign up for GitHub

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

[DevOverlay] Align old and new overlay #74935

Merged
merged 1 commit into from
Jan 23, 2025
+1,134 −654
Merged

[DevOverlay] Align old and new overlay #74935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions .github/workflows/build_and_test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ jobs:
uses: ./.github/workflows/build_reusable.yml
with:
nodeVersion: 18.18.2
afterBuild: __NEXT_EXPERIMENTAL_PPR=true NEXT_EXTERNAL_TESTS_FILTERS="test/ppr-tests-manifest.json" node run-tests.js --timings -c ${TEST_CONCURRENCY} --type integration
afterBuild: __NEXT_EXPERIMENTAL_PPR=true __NEXT_EXPERIMENTAL_NEW_DEV_OVERLAY=true NEXT_EXTERNAL_TESTS_FILTERS="test/ppr-tests-manifest.json" node run-tests.js --timings -c ${TEST_CONCURRENCY} --type integration
stepName: 'test-ppr-integration'
secrets: inherit

Expand All @@ -593,7 +593,7 @@ jobs:
group: [1/6, 2/6, 3/6, 4/6, 5/6, 6/6]
uses: ./.github/workflows/build_reusable.yml
with:
afterBuild: __NEXT_EXPERIMENTAL_PPR=true NEXT_EXTERNAL_TESTS_FILTERS="test/ppr-tests-manifest.json" NEXT_TEST_MODE=dev node run-tests.js --timings -g ${{ matrix.group }} -c ${TEST_CONCURRENCY} --type development
afterBuild: __NEXT_EXPERIMENTAL_PPR=true __NEXT_EXPERIMENTAL_NEW_DEV_OVERLAY=true NEXT_EXTERNAL_TESTS_FILTERS="test/ppr-tests-manifest.json" NEXT_TEST_MODE=dev node run-tests.js --timings -g ${{ matrix.group }} -c ${TEST_CONCURRENCY} --type development
stepName: 'test-ppr-dev-${{ matrix.group }}'
secrets: inherit

Expand All @@ -608,7 +608,7 @@ jobs:
group: [1/7, 2/7, 3/7, 4/7, 5/7, 6/7, 7/7]
uses: ./.github/workflows/build_reusable.yml
with:
afterBuild: __NEXT_EXPERIMENTAL_PPR=true NEXT_EXTERNAL_TESTS_FILTERS="test/ppr-tests-manifest.json" NEXT_TEST_MODE=start node run-tests.js --timings -g ${{ matrix.group }} -c ${TEST_CONCURRENCY} --type production
afterBuild: __NEXT_EXPERIMENTAL_PPR=true __NEXT_EXPERIMENTAL_NEW_DEV_OVERLAY=true NEXT_EXTERNAL_TESTS_FILTERS="test/ppr-tests-manifest.json" NEXT_TEST_MODE=start node run-tests.js --timings -g ${{ matrix.group }} -c ${TEST_CONCURRENCY} --type production
stepName: 'test-ppr-prod-${{ matrix.group }}'
secrets: inherit

Expand Down
6 changes: 5 additions & 1 deletion packages/next/src/build/webpack/plugins/define-env-plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,11 @@ export function getDefineEnv({
}
: undefined),
'process.env.__NEXT_EXPERIMENTAL_NEW_DEV_OVERLAY':
config.experimental.newDevOverlay ?? false,
// When `__NEXT_EXPERIMENTAL_NEW_DEV_OVERLAY` is set on CI,
// we need to pass it here so it can be enabled.
process.env.__NEXT_EXPERIMENTAL_NEW_DEV_OVERLAY === 'true' ||
Copy link
Member

Choose a reason for hiding this comment

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

why we need another flag __NEXT_EXPERIMENTAL_NEW_DEV_OVERLAY set in CI, shouldn't it just be __NEXT_EXPERIMENTAL_PPR === true? the variable on nextjs side is all set by define plugin

Copy link
Member Author

Choose a reason for hiding this comment

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

x-ref: #74935 (comment)

__NEXT_EXPERIMENTAL_PPR is feasible as it's only for testing, but I think explicit __NEXT_EXPERIMENTAL_NEW_DEV_OVERLAY would be better to isolate the condition in case needed.

config.experimental.newDevOverlay ||
false,
}

const userDefines = config.compiler?.define ?? {}
Expand Down
28 changes: 17 additions & 11 deletions packages/next/src/client/components/react-dev-overlay/_experimental/app/error-boundary.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@ import type { GlobalErrorComponent } from '../../../error-boundary'
import { PureComponent } from 'react'
import { RuntimeErrorHandler } from '../../../errors/runtime-error-handler'

type DevToolsErrorBoundaryProps = {
type DevOverlayErrorBoundaryProps = {
children: React.ReactNode
onError: (value: boolean) => void
devOverlay: React.ReactNode
globalError: [GlobalErrorComponent, React.ReactNode]
onError: (value: boolean) => void
}

type DevToolsErrorBoundaryState = {
type DevOverlayErrorBoundaryState = {
isReactError: boolean
reactError: unknown
}
Expand Down Expand Up @@ -37,9 +38,9 @@ function ErroredHtml({
)
}

export class DevToolsErrorBoundary extends PureComponent<
DevToolsErrorBoundaryProps,
DevToolsErrorBoundaryState
export class DevOverlayErrorBoundary extends PureComponent<
DevOverlayErrorBoundaryProps,
DevOverlayErrorBoundaryState
> {
state = { isReactError: false, reactError: null }

Expand All @@ -61,13 +62,18 @@ export class DevToolsErrorBoundary extends PureComponent<
}

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

const fallback = (
<ErroredHtml
globalError={this.props.globalError}
error={this.state.reactError}
/>
<ErroredHtml globalError={globalError} error={reactError} />
)

return this.state.isReactError ? fallback : this.props.children
return (
<>
{isReactError ? fallback : children}
{devOverlay}
</>
)
}
}
55 changes: 28 additions & 27 deletions ...ages/next/src/client/components/react-dev-overlay/_experimental/app/react-dev-overlay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { OverlayState } from '../../shared'
import type { GlobalErrorComponent } from '../../../error-boundary'

import { useState } from 'react'
import { DevToolsErrorBoundary } from './error-boundary'
import { DevOverlayErrorBoundary } from './error-boundary'
import { ShadowPortal } from '../internal/components/shadow-portal'
import { Base } from '../internal/styles/base'
import { ComponentStyles } from '../internal/styles/component-styles'
Expand All @@ -24,34 +24,35 @@ export default function ReactDevOverlay({
const [isErrorOverlayOpen, setIsErrorOverlayOpen] = useState(false)
const { readyErrors } = useErrorHook({ errors: state.errors, isAppDir: true })

return (
<>
<DevToolsErrorBoundary
onError={setIsErrorOverlayOpen}
globalError={globalError}
>
{children}
</DevToolsErrorBoundary>
const devOverlay = (
<ShadowPortal>
<CssReset />
<Base />
<Colors />
<ComponentStyles />

<ShadowPortal>
<CssReset />
<Base />
<Colors />
<ComponentStyles />
<DevToolsIndicator
state={state}
readyErrorsLength={readyErrors.length}
setIsErrorOverlayOpen={setIsErrorOverlayOpen}
/>

<DevToolsIndicator
state={state}
readyErrorsLength={readyErrors.length}
setIsErrorOverlayOpen={setIsErrorOverlayOpen}
/>
<ErrorOverlay
state={state}
readyErrors={readyErrors}
isErrorOverlayOpen={isErrorOverlayOpen}
setIsErrorOverlayOpen={setIsErrorOverlayOpen}
/>
</ShadowPortal>
)

<ErrorOverlay
state={state}
readyErrors={readyErrors}
isErrorOverlayOpen={isErrorOverlayOpen}
setIsErrorOverlayOpen={setIsErrorOverlayOpen}
/>
</ShadowPortal>
</>
return (
<DevOverlayErrorBoundary
devOverlay={devOverlay}
globalError={globalError}
onError={setIsErrorOverlayOpen}
>
{children}
</DevOverlayErrorBoundary>
)
}
4 changes: 1 addition & 3 deletions .../components/react-dev-overlay/_experimental/internal/components/code-frame/code-frame.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,7 @@ export function CodeFrame({ stackFrame, codeFrame }: CodeFrameProps) {
.map((line, a) =>
~(a = line.indexOf('|'))
? line.substring(0, a) +
line
.substring(a + 1)
.replace(`^\\ {${miniLeadingSpacesLength}}`, '')
line.substring(a).replace(`^\\ {${miniLeadingSpacesLength}}`, '')
Copy link
Member Author

Choose a reason for hiding this comment

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

Don't remove | from the code frame after the line number, as it increases too many conditional snapshot testings. This is the original behavior from the old UI.

next.js/packages/next/src/client/components/react-dev-overlay/internal/components/CodeFrame/CodeFrame.tsx

Line 37 in ab87453

line.substring(a).replace(`^\\ {${miniLeadingSpacesLength}}`, '')

: line
)
.join('\n')
Expand Down
7 changes: 5 additions & 2 deletions ...rlay/_experimental/internal/components/errors/dev-tools-indicator/dev-tools-indicator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ const DevToolsPopover = ({

return (
<Toast
data-nextjs-toast
style={{
boxShadow: 'none',
zIndex: 2147483647,
Expand Down Expand Up @@ -174,6 +175,7 @@ const DevToolsPopover = ({
onClick={hide}
/>
<IndicatorRow
data-nextjs-route-type={isStaticRoute ? 'static' : 'dynamic'}
label="Route"
value={isStaticRoute ? 'Static' : 'Dynamic'}
/>
Expand Down Expand Up @@ -205,14 +207,15 @@ const IndicatorRow = ({
label,
value,
onClick,
...props
}: {
label: string
value: React.ReactNode
onClick?: () => void
}) => {
} & React.HTMLAttributes<HTMLDivElement | HTMLButtonElement>) => {
const Wrapper = onClick ? 'button' : 'div'
return (
<Wrapper data-nextjs-dev-tools-row onClick={onClick}>
<Wrapper data-nextjs-dev-tools-row onClick={onClick} {...props}>
<span data-nextjs-dev-tools-row-label>{label}</span>
<span data-nextjs-dev-tools-row-value>{value}</span>
</Wrapper>
Expand Down
3 changes: 2 additions & 1 deletion ...erlay/_experimental/internal/components/errors/dev-tools-indicator/internal/next-logo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,8 @@ export const NextLogo = ({
aria-label="Open issues overlay"
onClick={onIssuesClick}
>
{issueCount} {issueCount === 1 ? 'Issue' : 'Issues'}
<span data-issues-count>{issueCount}</span>{' '}
{issueCount === 1 ? 'Issue' : 'Issues'}
</button>
<button
data-issues-collapse
Expand Down
4 changes: 2 additions & 2 deletions ...ient/components/react-dev-overlay/_experimental/internal/components/terminal/terminal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ export const Terminal: React.FC<TerminalProps> = function Terminal({
>
<span>
<FileIcon />
{getFrameSource(stackFrame)} @{' '}
<HotlinkedText text={stackFrame.methodName} />
{getFrameSource(stackFrame)}
{/* TODO: Unlike the CodeFrame component, the `methodName` is unavailable. */}
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove unnecessary @ from the Terminal component, as we won't know the method name from the build error. This is the behavior from the old UI.

next.js/packages/next/src/client/components/react-dev-overlay/internal/components/Terminal/Terminal.tsx

Lines 75 to 82 in ab87453

{file && (
<EditorLink
isSourceFile
key={file.fileName}
file={file.fileName}
location={file.location}
/>
)}

</span>
<ExternalIcon width={16} height={16} />
</p>
Expand Down
10 changes: 5 additions & 5 deletions packages/next/src/client/components/react-dev-overlay/_experimental/pages/error-boundary.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
import * as React from 'react'

type DevToolsErrorBoundaryProps = {
type DevOverlayErrorBoundaryProps = {
children?: React.ReactNode
onError: (error: Error, componentStack: string | null) => void
isMounted?: boolean
}
type DevToolsErrorBoundaryState = { error: Error | null }
type DevOverlayErrorBoundaryState = { error: Error | null }

export class DevToolsErrorBoundary extends React.PureComponent<
DevToolsErrorBoundaryProps,
DevToolsErrorBoundaryState
export class DevOverlayErrorBoundary extends React.PureComponent<
DevOverlayErrorBoundaryProps,
DevOverlayErrorBoundaryState
> {
state = { error: null }

Expand Down
63 changes: 36 additions & 27 deletions ...es/next/src/client/components/react-dev-overlay/_experimental/pages/react-dev-overlay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { Base } from '../internal/styles/base'
import { ComponentStyles } from '../internal/styles/component-styles'
import { CssReset } from '../internal/styles/css-reset'

import { DevToolsErrorBoundary } from './error-boundary'
import { DevOverlayErrorBoundary } from './error-boundary'
import { usePagesReactDevOverlay } from '../../pages/hooks'
import { Colors } from '../internal/styles/colors'
import { ErrorOverlay } from '../internal/components/errors/error-overlay/error-overlay'
Expand All @@ -20,41 +20,50 @@ interface ReactDevOverlayProps {
}

export default function ReactDevOverlay({ children }: ReactDevOverlayProps) {
const { isMounted, state, onComponentError, hasRuntimeErrors } =
usePagesReactDevOverlay()

const [isErrorOverlayOpen, setIsErrorOverlayOpen] = useState(hasRuntimeErrors)
const {
isMounted,
state,
onComponentError,
hasRuntimeErrors,
hasBuildError,
} = usePagesReactDevOverlay()

const { readyErrors } = useErrorHook({
errors: state.errors,
isAppDir: false,
})

const [isErrorOverlayOpen, setIsErrorOverlayOpen] = useState(true)
Copy link
Member Author

Choose a reason for hiding this comment

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

Set default value of isErrorOverlayOpen to true. This is the behavior from the old UI.

next.js/packages/next/src/client/components/react-dev-overlay/pages/old-react-dev-overlay.tsx

Line 48 in ab87453

initialDisplayState={'fullscreen'}


return (
<>
<DevToolsErrorBoundary isMounted={isMounted} onError={onComponentError}>
<DevOverlayErrorBoundary isMounted={isMounted} onError={onComponentError}>
{children ?? null}
</DevToolsErrorBoundary>

<ShadowPortal>
<CssReset />
<Base />
<Colors />
<ComponentStyles />

<DevToolsIndicator
state={state}
readyErrorsLength={readyErrors.length}
setIsErrorOverlayOpen={setIsErrorOverlayOpen}
/>

<ErrorOverlay
state={state}
readyErrors={readyErrors}
isErrorOverlayOpen={isErrorOverlayOpen}
setIsErrorOverlayOpen={setIsErrorOverlayOpen}
/>
</ShadowPortal>
</DevOverlayErrorBoundary>

{isMounted && (
<ShadowPortal>
<CssReset />
<Base />
<Colors />
<ComponentStyles />

<DevToolsIndicator
state={state}
readyErrorsLength={readyErrors.length}
setIsErrorOverlayOpen={setIsErrorOverlayOpen}
/>

{(hasRuntimeErrors || hasBuildError) && (
<ErrorOverlay
state={state}
readyErrors={readyErrors}
isErrorOverlayOpen={isErrorOverlayOpen}
setIsErrorOverlayOpen={setIsErrorOverlayOpen}
/>
)}
</ShadowPortal>
)}
</>
)
}
2 changes: 1 addition & 1 deletion ...es/next/src/client/components/react-dev-overlay/internal/container/RuntimeError/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export function RuntimeError({ error }: RuntimeErrorProps) {
? []
: filteredFrames.slice(0, firstFirstPartyFrameIndex),
trailingCallStackFrames: filteredFrames.slice(
firstFirstPartyFrameIndex + 1
firstFirstPartyFrameIndex < 0 ? 0 : firstFirstPartyFrameIndex
Copy link
Member Author

@devjiwonchoi devjiwonchoi Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is the old UI, backporting the behavior to include the first first-party call stack frame to be included in the CallStack component. Previously, it was only displayed at the CodeFrame.

next.js/packages/next/src/client/components/react-dev-overlay/_experimental/internal/container/runtime-error/index.tsx

Lines 12 to 23 in ab87453

const { firstFrame } = useMemo(() => {
const firstFirstPartyFrameIndex = error.frames.findIndex(
(entry) =>
!entry.ignored &&
Boolean(entry.originalCodeFrame) &&
Boolean(entry.originalStackFrame)
)
return {
firstFrame: error.frames[firstFirstPartyFrameIndex] ?? null,
}
}, [error.frames])

),
}
}, [error.frames, isIgnoredExpanded])
Expand Down
Loading
Loading