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(error-overlay): correct module grouping #62206

Merged
merged 21 commits into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from 20 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 @@ -5,62 +5,56 @@ import { FrameworkIcon } from './FrameworkIcon'
function FrameworkGroup({
framework,
stackFrames,
all,
}: {
framework: NonNullable<StackFramesGroup['framework']>
stackFrames: StackFramesGroup['stackFrames']
all: boolean
}) {
return (
<>
<details data-nextjs-collapsed-call-stack-details>
<summary
tabIndex={10} // Match CallStackFrame tabIndex
<details data-nextjs-collapsed-call-stack-details>
{/* Match CallStackFrame tabIndex */}
<summary tabIndex={10}>
<svg
data-nextjs-call-stack-chevron-icon
fill="none"
height="20"
width="20"
shapeRendering="geometricPrecision"
stroke="currentColor"
strokeLinecap="round"
strokeLinejoin="round"
strokeWidth="2"
viewBox="0 0 24 24"
>
<svg
data-nextjs-call-stack-chevron-icon
fill="none"
height="20"
width="20"
shapeRendering="geometricPrecision"
stroke="currentColor"
strokeLinecap="round"
strokeLinejoin="round"
strokeWidth="2"
viewBox="0 0 24 24"
>
<path d="M9 18l6-6-6-6" />
</svg>
<FrameworkIcon framework={framework} />
{framework === 'react' ? 'React' : 'Next.js'}
</summary>

{stackFrames.map((frame, index) => (
<CallStackFrame key={`call-stack-${index}-${all}`} frame={frame} />
))}
</details>
</>
<path d="M9 18l6-6-6-6" />
</svg>
<FrameworkIcon framework={framework} />
{framework === 'react' ? 'React' : 'Next.js'}
</summary>
{stackFrames.map((frame, index) => (
<CallStackFrame key={`call-stack-${index}`} frame={frame} />
))}
</details>
)
}

export function GroupedStackFrames({
groupedStackFrames,
all,
show,
}: {
groupedStackFrames: StackFramesGroup[]
all: boolean
show: boolean
}) {
if (!show) return
Copy link
Member Author

Choose a reason for hiding this comment

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

We should respect the main "Hide/Show collapsed frames" and should not show grouped stackframes at all when collapsed

return (
<>
{groupedStackFrames.map((stackFramesGroup, groupIndex) => {
// Collapse React and Next.js frames
if (stackFramesGroup.framework) {
return (
<FrameworkGroup
key={`call-stack-framework-group-${groupIndex}-${all}`}
key={`call-stack-framework-group-${groupIndex}`}
framework={stackFramesGroup.framework}
stackFrames={stackFramesGroup.stackFrames}
all={all}
/>
)
}
Expand All @@ -69,7 +63,7 @@ export function GroupedStackFrames({
// Don't group non React and Next.js frames
stackFramesGroup.stackFrames.map((frame, frameIndex) => (
<CallStackFrame
key={`call-stack-${groupIndex}-${frameIndex}-${all}`}
key={`call-stack-${groupIndex}-${frameIndex}`}
frame={frame}
/>
))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import type { ReadyRuntimeError } from '../../helpers/getErrorByType'
import { noop as css } from '../../helpers/noop-template'
import type { OriginalStackFrame } from '../../helpers/stack-frame'
import { groupStackFramesByFramework } from '../../helpers/group-stack-frames-by-framework'
import { CallStackFrame } from './CallStackFrame'
import { GroupedStackFrames } from './GroupedStackFrames'
import { ComponentStackFrameRow } from './ComponentStackFrameRow'

Expand Down Expand Up @@ -64,21 +63,24 @@ const RuntimeError: React.FC<RuntimeErrorProps> = function RuntimeError({
])

const stackFramesGroupedByFramework = React.useMemo(
() => groupStackFramesByFramework(visibleCallStackFrames),
[visibleCallStackFrames]
() => groupStackFramesByFramework(allCallStackFrames),
[allCallStackFrames]
)

const leadingFramesGroupedByFramework = React.useMemo(
() => groupStackFramesByFramework(leadingFrames),
[leadingFrames]
)

return (
<React.Fragment>
{firstFrame ? (
<React.Fragment>
<h2>Source</h2>
{leadingFrames.map((frame, index) => (
<CallStackFrame
key={`leading-frame-${index}-${all}`}
frame={frame}
/>
))}
<GroupedStackFrames
groupedStackFrames={leadingFramesGroupedByFramework}
show={all}
/>
<CodeFrame
stackFrame={firstFrame.originalStackFrame!}
codeFrame={firstFrame.originalCodeFrame!}
Expand All @@ -103,7 +105,7 @@ const RuntimeError: React.FC<RuntimeErrorProps> = function RuntimeError({
<h2>Call Stack</h2>
<GroupedStackFrames
groupedStackFrames={stackFramesGroupedByFramework}
all={all}
show={all}
/>
</React.Fragment>
) : undefined}
Expand Down Expand Up @@ -196,7 +198,7 @@ export const styles = css`
[data-nextjs-collapsed-call-stack-details] summary {
display: flex;
align-items: center;
margin: var(--size-gap-double) 0;
margin-bottom: var(--size-gap);
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed some inconsistent spacing, here is the before/after. (it makes the stack shorter as well):
image image

list-style: none;
}
[data-nextjs-collapsed-call-stack-details] summary::-webkit-details-marker {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,31 +1,11 @@
import type { SourcePackage } from '../../server/shared'
import type { OriginalStackFrame } from './stack-frame'

export type StackFramesGroup = {
framework?: 'next' | 'react'
framework?: SourcePackage | null
stackFrames: OriginalStackFrame[]
}

/**
* Get the origin framework of the stack frame by package name.
*/
function getFramework(
sourcePackage: string | undefined
): StackFramesGroup['framework'] {
if (!sourcePackage) return undefined

if (
/^(react|react-dom|react-is|react-refresh|react-server-dom-webpack|react-server-dom-turbopack|scheduler)$/.test(
sourcePackage
)
) {
return 'react'
} else if (sourcePackage === 'next') {
return 'next'
}

return undefined
}
balazsorban44 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Group sequences of stack frames by framework.
*
Expand Down Expand Up @@ -55,7 +35,7 @@ export function groupStackFramesByFramework(
for (const stackFrame of stackFrames) {
const currentGroup =
stackFramesGroupedByFramework[stackFramesGroupedByFramework.length - 1]
const framework = getFramework(stackFrame.sourcePackage)
const framework = stackFrame.sourcePackage

if (currentGroup && currentGroup.framework === framework) {
currentGroup.stackFrames.push(stackFrame)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,37 +1,13 @@
import type { StackFrame } from 'next/dist/compiled/stacktrace-parser'
import type { OriginalStackFrameResponse } from '../../server/middleware'
import type { OriginalStackFrameResponse } from '../../server/shared'

export type OriginalStackFrame =
| {
error: true
reason: string
external: false
expanded: false
sourceStackFrame: StackFrame
originalStackFrame: null
originalCodeFrame: null
sourcePackage?: string
}
| {
error: false
reason: null
external: false
expanded: boolean
sourceStackFrame: StackFrame
originalStackFrame: StackFrame
originalCodeFrame: string | null
sourcePackage?: string
}
| {
error: false
reason: null
external: true
expanded: false
sourceStackFrame: StackFrame
originalStackFrame: null
originalCodeFrame: null
sourcePackage?: string
}
export interface OriginalStackFrame extends OriginalStackFrameResponse {
error: boolean
reason: string | null
external: boolean
expanded: boolean
sourceStackFrame: StackFrame
}

function getOriginalStackFrame(
source: StackFrame,
Expand Down Expand Up @@ -99,6 +75,7 @@ function getOriginalStackFrame(
sourceStackFrame: source,
originalStackFrame: null,
originalCodeFrame: null,
sourcePackage: null,
})
}

Expand All @@ -110,6 +87,7 @@ function getOriginalStackFrame(
sourceStackFrame: source,
originalStackFrame: null,
originalCodeFrame: null,
sourcePackage: null,
}))
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import type { IncomingMessage, ServerResponse } from 'http'
import type { ParsedUrlQuery } from 'querystring'
import type { OriginalStackFrameResponse } from './middleware'
import { findSourcePackage, type OriginalStackFrameResponse } from './shared'

import fs, { constants as FS } from 'fs/promises'
import url from 'url'
import { codeFrameColumns } from 'next/dist/compiled/babel/code-frame'
import { launchEditor } from '../internal/helpers/launchEditor'

Expand Down Expand Up @@ -76,6 +74,8 @@ export async function createOriginalStackFrame(
): Promise<OriginalStackFrameResponse | null> {
const traced = await batchedTraceSource(project, frame)
if (!traced) {
const sourcePackage = findSourcePackage(frame.file)
if (sourcePackage) return { sourcePackage }
return null
}

Expand All @@ -96,27 +96,24 @@ export async function createOriginalStackFrame(
},
{ forceColor: true }
),
}
}

function stackFrameFromQuery(query: ParsedUrlQuery): TurbopackStackFrame {
return {
file: query.file as string,
methodName: query.methodName as string,
line:
typeof query.lineNumber === 'string' ? parseInt(query.lineNumber, 10) : 0,
column:
typeof query.column === 'string' ? parseInt(query.column, 10) : null,
isServer: query.isServer === 'true',
sourcePackage: findSourcePackage(traced.frame.file),
}
}

export function getOverlayMiddleware(project: Project) {
return async function (req: IncomingMessage, res: ServerResponse) {
const { pathname, query } = url.parse(req.url!, true)
const { pathname, searchParams } = new URL(req.url!, 'http://n')

const frame = {
balazsorban44 marked this conversation as resolved.
Show resolved Hide resolved
file: searchParams.get('file') as string,
methodName: searchParams.get('methodName'),
line: parseInt(searchParams.get('lineNumber') ?? '0', 10) || 0,
column: parseInt(searchParams.get('column') ?? '0', 10) || 0,
isServer: searchParams.get('isServer') === 'true',
} satisfies TurbopackStackFrame

if (pathname === '/__nextjs_original-stack-frame') {
const frame = stackFrameFromQuery(query)
let originalStackFrame
let originalStackFrame: Partial<OriginalStackFrameResponse> | null
balazsorban44 marked this conversation as resolved.
Show resolved Hide resolved
try {
originalStackFrame = await createOriginalStackFrame(project, frame)
} catch (e: any) {
Expand All @@ -139,17 +136,14 @@ export function getOverlayMiddleware(project: Project) {
res.end()
return
} else if (pathname === '/__nextjs_launch-editor') {
const frame = stackFrameFromQuery(query)

const filePath = frame.file?.toString()
if (filePath === undefined) {
if (!frame.file) {
res.statusCode = 400
res.write('Bad Request')
res.end()
return
}

const fileExists = await fs.access(filePath, FS.F_OK).then(
const fileExists = await fs.access(frame.file, FS.F_OK).then(
() => true,
() => false
)
Expand All @@ -161,7 +155,7 @@ export function getOverlayMiddleware(project: Project) {
}

try {
launchEditor(filePath, frame.line ?? 1, frame.column ?? 1)
launchEditor(frame.file, frame.line ?? 1, frame.column ?? 1)
} catch (err) {
console.log('Failed to launch editor:', err)
res.statusCode = 500
Expand Down
Loading
Loading