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

[dev-overlay] Fix off-by-one column sourcemapping in Webpack #76152

Merged
merged 1 commit into from
Feb 18, 2025
Merged

[dev-overlay] Fix off-by-one column sourcemapping in Webpack #76152

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
3 changes: 2 additions & 1 deletion packages/next/src/build/webpack/plugins/wellknown-errors-plugin/parseNotFoundError.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ async function getSourceFrame(
file: fileName,
methodName: '',
lineNumber: loc.start.line,
column: loc.start.column,
// loc is 0-based but columns in stack frames are 1-based.
column: (loc.start.column ?? 0) + 1,
},
})

Expand Down
6 changes: 5 additions & 1 deletion packages/next/src/client/components/react-dev-overlay/server/middleware-turbopack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,9 @@ function findApplicableSourceMapPayload(
}
}

/**
* @returns 1-based lines and 0-based columns
*/
async function nativeTraceSource(
frame: TurbopackStackFrame
): Promise<{ frame: IgnorableStackFrame; source: string | null } | undefined> {
Expand Down Expand Up @@ -264,7 +267,8 @@ async function nativeTraceSource(
try {
const originalPosition = consumer.originalPositionFor({
line: frame.line ?? 1,
column: frame.column ?? 1,
// 0-based columns out requires 0-based columns in.
column: (frame.column ?? 1) - 1,
})

if (originalPosition.source === null) {
Expand Down
17 changes: 8 additions & 9 deletions packages/next/src/client/components/react-dev-overlay/server/middleware-webpack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,12 @@ function getSourcePath(source: string) {
return source.replace(/^(webpack:\/\/\/|webpack:\/\/|webpack:\/\/_N_E\/)/, '')
}

/**
* @returns 1-based lines and 0-based columns
*/
async function findOriginalSourcePositionAndContent(
sourceMap: RawSourceMap,
position: { line: number; column: number | null }
position: { lineNumber: number | null; column: number | null }
): Promise<SourceAttributes | null> {
let consumer: BasicSourceMapConsumer
try {
Expand All @@ -106,8 +109,9 @@ async function findOriginalSourcePositionAndContent(

try {
const sourcePosition = consumer.originalPositionFor({
line: position.line,
column: position.column ?? 0,
line: position.lineNumber ?? 1,
// 0-based columns out requires 0-based columns in.
column: (position.column ?? 1) - 1,
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 the actual fix. The rest just makes this middleware more similar to the Turbopack middleware while also creating less intermediate objects.

})

if (!sourcePosition.source) {
Expand Down Expand Up @@ -191,7 +195,6 @@ export async function createOriginalStackFrame({
frame: StackFrame
errorMessage?: string
}): Promise<OriginalStackFrameResponse | null> {
const { lineNumber, column } = frame
const moduleNotFound = findModuleNotFoundFromError(errorMessage)
const result = await (() => {
if (moduleNotFound) {
Expand All @@ -205,11 +208,7 @@ export async function createOriginalStackFrame({
source.compilation
)
}
// This returns 1-based lines and 0-based columns
return findOriginalSourcePositionAndContent(source.sourceMap, {
line: lineNumber ?? 1,
column,
})
return findOriginalSourcePositionAndContent(source.sourceMap, frame)
})()

if (!result) {
Expand Down
96 changes: 23 additions & 73 deletions ...pment/app-dir/capture-console-error-owner-stack/capture-console-error-owner-stack.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,54 +41,28 @@ describe('app-dir - capture-console-error-owner-stack', () => {

const result = await getRedboxResult(browser)

// TODO(veil): Inconsistent cursor position for the "Page" frame
if (process.env.TURBOPACK) {
expect(result).toMatchInlineSnapshot(`
{
"callStacks": "onClick
app/browser/event/page.js (7:17)
button
<anonymous> (0:0)
Page
app/browser/event/page.js (5:5)",
"count": 1,
"description": "trigger an console <error>",
"source": "app/browser/event/page.js (7:17) @ onClick

5 | <button
6 | onClick={() => {
> 7 | console.error('trigger an console <%s>', 'error')
| ^
8 | }}
9 | >
10 | click to error",
"title": "Console Error",
}
`)
} else {
expect(result).toMatchInlineSnapshot(`
{
"callStacks": "onClick
app/browser/event/page.js (7:17)
button
<anonymous> (0:0)
Page
app/browser/event/page.js (5:6)",
"count": 1,
"description": "trigger an console <error>",
"source": "app/browser/event/page.js (7:17) @ onClick

5 | <button
6 | onClick={() => {
> 7 | console.error('trigger an console <%s>', 'error')
| ^
8 | }}
9 | >
10 | click to error",
"title": "Console Error",
}
`)
}
expect(result).toMatchInlineSnapshot(`
{
"callStacks": "onClick
app/browser/event/page.js (7:17)
button
<anonymous> (0:0)
Page
app/browser/event/page.js (5:5)",
"count": 1,
"description": "trigger an console <error>",
"source": "app/browser/event/page.js (7:17) @ onClick

5 | <button
6 | onClick={() => {
> 7 | console.error('trigger an console <%s>', 'error')
| ^
8 | }}
9 | >
10 | click to error",
"title": "Console Error",
}
`)
})

it('should capture browser console error in render and dedupe if necessary', async () => {
Expand Down Expand Up @@ -227,30 +201,7 @@ describe('app-dir - capture-console-error-owner-stack', () => {
}
`)
} else {
if (process.env.TURBOPACK) {
expect(result).toMatchInlineSnapshot(`
{
"callStacks": "Page
app/rsc/page.js (2:17)
JSON.parse
<anonymous> (0:0)
Page
<anonymous> (0:0)",
"count": 1,
"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 |",
"title": "Console Error",
}
`)
} else {
expect(result).toMatchInlineSnapshot(`
expect(result).toMatchInlineSnapshot(`
{
"callStacks": "Page
app/rsc/page.js (2:17)
Expand All @@ -271,7 +222,6 @@ describe('app-dir - capture-console-error-owner-stack', () => {
"title": "Console Error",
}
`)
}
}
})

Expand Down
44 changes: 22 additions & 22 deletions test/development/app-dir/error-overlay/error-ignored-frames/error-ignored-frames.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
} from 'next-test-utils'

describe('error-ignored-frames', () => {
const { next } = nextTestSetup({
const { isTurbopack, next } = nextTestSetup({
files: __dirname,
})

Expand All @@ -30,7 +30,7 @@ describe('error-ignored-frames', () => {
await toggleCollapseCallStackFrames(browser)

const expendedStack = await getStackFramesContent(browser)
if (process.env.TURBOPACK) {
if (isTurbopack) {
expect(expendedStack).toMatchInlineSnapshot(`
"at Page (app/page.tsx (2:9))
at resolveErrorDev ()
Expand All @@ -50,11 +50,11 @@ describe('error-ignored-frames', () => {
at processFullStringRow ()
at processFullBinaryRow ()
at progress ()
at InnerLayoutRouter (../src/client/components/layout-router.tsx (408:6))
at OuterLayoutRouter (../src/client/components/layout-router.tsx (607:20))
at Router (../src/client/components/app-router.tsx (633:8))
at AppRouter (../src/client/components/app-router.tsx (679:8))
at ServerRoot (../src/client/app-index.tsx (201:6))"
at InnerLayoutRouter (../src/client/components/layout-router.tsx (408:5))
at OuterLayoutRouter (../src/client/components/layout-router.tsx (607:19))
at Router (../src/client/components/app-router.tsx (633:7))
at AppRouter (../src/client/components/app-router.tsx (679:7))
at ServerRoot (../src/client/app-index.tsx (201:5))"
`)
}
})
Expand All @@ -71,7 +71,7 @@ describe('error-ignored-frames', () => {
await toggleCollapseCallStackFrames(browser)

const expendedStack = await getStackFramesContent(browser)
if (process.env.TURBOPACK) {
if (isTurbopack) {
expect(expendedStack).toMatchInlineSnapshot(`
"at Page (app/client/page.tsx (4:9))
at ClientPageRoot ()
Expand All @@ -82,10 +82,10 @@ describe('error-ignored-frames', () => {
} else {
expect(expendedStack).toMatchInlineSnapshot(`
"at Page (app/client/page.tsx (4:9))
at ClientPageRoot (../src/client/components/client-page.tsx (60:13))
at Router (../src/client/components/app-router.tsx (633:8))
at AppRouter (../src/client/components/app-router.tsx (679:8))
at ServerRoot (../src/client/app-index.tsx (201:6))"
at ClientPageRoot (../src/client/components/client-page.tsx (60:12))
at Router (../src/client/components/app-router.tsx (633:7))
at AppRouter (../src/client/components/app-router.tsx (679:7))
at ServerRoot (../src/client/app-index.tsx (201:5))"
`)
}
})
Expand All @@ -95,26 +95,26 @@ describe('error-ignored-frames', () => {
await assertHasRedbox(browser)

const defaultStack = await getStackFramesContent(browser)
if (process.env.TURBOPACK) {
if (isTurbopack) {
expect(defaultStack).toMatchInlineSnapshot(`
"at <unknown> (app/interleaved/page.tsx (7:11))
at Page (app/interleaved/page.tsx (6:35))"
`)
} else {
expect(defaultStack).toMatchInlineSnapshot(`
"at eval (app/interleaved/page.tsx (7:11))
at Page (app/interleaved/page.tsx (6:37))"
at Page (app/interleaved/page.tsx (6:36))"
`)
}

await toggleCollapseCallStackFrames(browser)

const expendedStack = await getStackFramesContent(browser)
if (process.env.TURBOPACK) {
if (isTurbopack) {
expect(expendedStack).toMatchInlineSnapshot(`
"at <unknown> (app/interleaved/page.tsx (7:11))
at invokeCallback ()
at Page (app/interleaved/page.tsx (6:35))
at invokeCallback ()
at ClientPageRoot ()
at Router ()
at AppRouter ()
Expand All @@ -124,11 +124,11 @@ describe('error-ignored-frames', () => {
expect(expendedStack).toMatchInlineSnapshot(`
"at eval (app/interleaved/page.tsx (7:11))
at invokeCallback (node_modules/interleave/index.js (2:1))
at Page (app/interleaved/page.tsx (6:37))
at ClientPageRoot (../src/client/components/client-page.tsx (60:13))
at Router (../src/client/components/app-router.tsx (633:8))
at AppRouter (../src/client/components/app-router.tsx (679:8))
at ServerRoot (../src/client/app-index.tsx (201:6))"
at Page (app/interleaved/page.tsx (6:36))
at ClientPageRoot (../src/client/components/client-page.tsx (60:12))
at Router (../src/client/components/app-router.tsx (633:7))
at AppRouter (../src/client/components/app-router.tsx (679:7))
at ServerRoot (../src/client/app-index.tsx (201:5))"
`)
}
})
Expand All @@ -145,7 +145,7 @@ describe('error-ignored-frames', () => {
await toggleCollapseCallStackFrames(browser)

const expendedStack = await getStackFramesContent(browser)
if (process.env.TURBOPACK) {
if (isTurbopack) {
expect(expendedStack).toMatchInlineSnapshot(`
"at Page (pages/pages.tsx (2:9))
at react-stack-bottom-frame ()
Expand Down
Loading
Loading