From 804c4a13fdd5433a5170e5e4296359c6e9f30c42 Mon Sep 17 00:00:00 2001 From: Sebastian Sebbie Silbermann Date: Mon, 3 Mar 2025 08:10:50 +0100 Subject: [PATCH] [dev-overlay] Always show relative paths Stackframes that are not sourcemapped used to show absolute paths when the input was also an absolute path. This looks off when it is mixed with frames that sourcemapped and we display relative paths. Since we also have an explicit indicator for failed sourcemapping, we don't need to keep absolute file paths as a proxy. --- .../server/middleware-turbopack.ts | 41 ++++++---- .../server/middleware-webpack.ts | 12 ++- .../src/server/dev/hot-reloader-turbopack.ts | 16 ++-- .../acceptance-app/ReactRefreshLogBox.test.ts | 36 ++++----- .../acceptance/ReactRefreshLogBox.test.ts | 75 ++++++++++--------- .../acceptance/error-recovery.test.ts | 58 +++++++------- .../pages-dir/client-navigation/index.test.ts | 8 +- .../non-root-project-monorepo.test.ts | 16 ++-- 8 files changed, 147 insertions(+), 115 deletions(-) diff --git a/packages/next/src/client/components/react-dev-overlay/server/middleware-turbopack.ts b/packages/next/src/client/components/react-dev-overlay/server/middleware-turbopack.ts index f5b1ec6004366..086c8f1ed212d 100644 --- a/packages/next/src/client/components/react-dev-overlay/server/middleware-turbopack.ts +++ b/packages/next/src/client/components/react-dev-overlay/server/middleware-turbopack.ts @@ -34,7 +34,7 @@ function shouldIgnorePath(modulePath: string): boolean { type IgnorableStackFrame = StackFrame & { ignored: boolean } const currentSourcesByFile: Map> = new Map() -export async function batchedTraceSource( +async function batchedTraceSource( project: Project, frame: TurbopackStackFrame ): Promise<{ frame: IgnorableStackFrame; source: string | null } | undefined> { @@ -316,9 +316,7 @@ async function nativeTraceSource( ?.replace('__WEBPACK_DEFAULT_EXPORT__', 'default') ?.replace('__webpack_exports__.', '') || '', column: (originalPosition.column ?? 0) + 1, - file: originalPosition.source?.startsWith('file://') - ? relativeToCwd(originalPosition.source) - : originalPosition.source, + file: originalPosition.source, lineNumber: originalPosition.line ?? 0, // TODO: c&p from async createOriginalStackFrame but why not frame.arguments? arguments: [], @@ -335,14 +333,9 @@ async function nativeTraceSource( return undefined } -function relativeToCwd(file: string): string { - const relPath = path.relative(process.cwd(), url.fileURLToPath(file)) - // TODO(sokra) include a ./ here to make it a relative path - return relPath -} - async function createOriginalStackFrame( project: Project, + projectPath: string, frame: TurbopackStackFrame ): Promise { const traced = @@ -354,13 +347,31 @@ async function createOriginalStackFrame( return null } + let normalizedStackFrameLocation = traced.frame.file + if ( + normalizedStackFrameLocation !== null && + normalizedStackFrameLocation.startsWith('file://') + ) { + normalizedStackFrameLocation = path.relative( + projectPath, + url.fileURLToPath(normalizedStackFrameLocation) + ) + } + return { - originalStackFrame: traced.frame, + originalStackFrame: { + arguments: traced.frame.arguments, + column: traced.frame.column, + file: normalizedStackFrameLocation, + ignored: traced.frame.ignored, + lineNumber: traced.frame.lineNumber, + methodName: traced.frame.methodName, + }, originalCodeFrame: getOriginalCodeFrame(traced.frame, traced.source), } } -export function getOverlayMiddleware(project: Project) { +export function getOverlayMiddleware(project: Project, projectPath: string) { return async function ( req: IncomingMessage, res: ServerResponse, @@ -387,7 +398,11 @@ export function getOverlayMiddleware(project: Project) { const result: OriginalStackFramesResponse = await Promise.all( stackFrames.map(async (frame) => { try { - const stackFrame = await createOriginalStackFrame(project, frame) + const stackFrame = await createOriginalStackFrame( + project, + projectPath, + frame + ) if (stackFrame === null) { return { status: 'rejected', diff --git a/packages/next/src/client/components/react-dev-overlay/server/middleware-webpack.ts b/packages/next/src/client/components/react-dev-overlay/server/middleware-webpack.ts index 1719b2029262d..db1b7851c49a4 100644 --- a/packages/next/src/client/components/react-dev-overlay/server/middleware-webpack.ts +++ b/packages/next/src/client/components/react-dev-overlay/server/middleware-webpack.ts @@ -471,9 +471,19 @@ async function getOriginalStackFrame({ }, }) + let defaultNormalizedStackFrameLocation = frame.file + if ( + defaultNormalizedStackFrameLocation !== null && + defaultNormalizedStackFrameLocation.startsWith('file://') + ) { + defaultNormalizedStackFrameLocation = path.relative( + rootDirectory, + fileURLToPath(defaultNormalizedStackFrameLocation) + ) + } // This stack frame is used for the one that couldn't locate the source or source mapped frame const defaultStackFrame: IgnorableStackFrame = { - file: frame.file, + file: defaultNormalizedStackFrameLocation, lineNumber: frame.lineNumber, column: frame.column ?? 1, methodName: frame.methodName, diff --git a/packages/next/src/server/dev/hot-reloader-turbopack.ts b/packages/next/src/server/dev/hot-reloader-turbopack.ts index 4b9029c9a6d58..c45e9a957a9c7 100644 --- a/packages/next/src/server/dev/hot-reloader-turbopack.ts +++ b/packages/next/src/server/dev/hot-reloader-turbopack.ts @@ -161,7 +161,7 @@ export async function createHotReloaderTurbopack( ): Promise { const dev = true const buildId = 'development' - const { nextConfig, dir } = opts + const { nextConfig, dir: projectPath } = opts const { loadBindings } = require('../../build/swc') as typeof import('../../build/swc') @@ -172,7 +172,7 @@ export async function createHotReloaderTurbopack( // works correctly. Normally `run-test` hides output so only will be visible when `--debug` flag is used. if (process.env.TURBOPACK && isTestMode) { require('console').log('Creating turbopack project', { - dir, + dir: projectPath, testMode: isTestMode, }) } @@ -207,14 +207,14 @@ export async function createHotReloaderTurbopack( const project = await bindings.turbo.createProject( { - projectPath: dir, + projectPath: projectPath, rootPath: opts.nextConfig.experimental.turbo?.root || opts.nextConfig.outputFileTracingRoot || - dir, + projectPath, distDir, nextConfig: opts.nextConfig, - jsConfig: await getTurbopackJsConfig(dir, nextConfig), + jsConfig: await getTurbopackJsConfig(projectPath, nextConfig), watch: { enable: dev, pollIntervalMs: nextConfig.watchOptions?.pollIntervalMs, @@ -244,7 +244,7 @@ export async function createHotReloaderTurbopack( } ) setBundlerFindSourceMapImplementation( - getSourceMapFromTurbopack.bind(null, project, dir) + getSourceMapFromTurbopack.bind(null, project, projectPath) ) opts.onDevServerCleanup?.(async () => { setBundlerFindSourceMapImplementation(() => undefined) @@ -634,7 +634,7 @@ export async function createHotReloaderTurbopack( ) const middlewares = [ - getOverlayMiddleware(project), + getOverlayMiddleware(project, projectPath), getSourceMapMiddleware(project), getNextErrorFeedbackMiddleware(opts.telemetry), getDevOverlayFontMiddleware(), @@ -957,7 +957,7 @@ export async function createHotReloaderTurbopack( > = definition ?? (await findPagePathData( - dir, + projectPath, inputPage, nextConfig.pageExtensions, opts.pagesDir, diff --git a/test/development/acceptance-app/ReactRefreshLogBox.test.ts b/test/development/acceptance-app/ReactRefreshLogBox.test.ts index dedf7daa4aaf8..e9b7142669d8e 100644 --- a/test/development/acceptance-app/ReactRefreshLogBox.test.ts +++ b/test/development/acceptance-app/ReactRefreshLogBox.test.ts @@ -115,15 +115,15 @@ describe('ReactRefreshLogBox app', () => { | ^", "stack": [ "eval index.js (3:7)", - "", - "", - "", - "", + "", + "", + "", + "", "eval ./app/page.js", - "", - "", - "", - "", + "", + "", + "", + "", ], } `) @@ -1468,10 +1468,10 @@ describe('ReactRefreshLogBox app', () => { "stack": [ "eval index.js (1:7)", "", - "", + "", "eval ./app/server/page.js", "", - "", + "", ], } `) @@ -1639,15 +1639,15 @@ export default function Home() { | ^", "stack": [ "eval app/utils.ts (1:7)", - "", - "", - "", - "", + "", + "", + "", + "", "eval ./app/page.js", - "", - "", - "", - "", + "", + "", + "", + "", ], } `) diff --git a/test/development/acceptance/ReactRefreshLogBox.test.ts b/test/development/acceptance/ReactRefreshLogBox.test.ts index 33fcebfb56a6c..9e9b11edb7420 100644 --- a/test/development/acceptance/ReactRefreshLogBox.test.ts +++ b/test/development/acceptance/ReactRefreshLogBox.test.ts @@ -131,9 +131,9 @@ describe('ReactRefreshLogBox', () => { | ^", "stack": [ "[project]/index.js [ssr] (ecmascript) index.js (3:7)", - "", - "", - "", + "", + "", + "", ], } `) @@ -149,18 +149,18 @@ describe('ReactRefreshLogBox', () => { | ^", "stack": [ "eval index.js (3:7)", - "", - "", + "", + "", "eval ./pages/index.js", - "", - "", - "", - "", - "", - "", - "", - "", - "", + "", + "", + "", + "", + "", + "", + "", + "", + "", ], } `) @@ -178,9 +178,9 @@ describe('ReactRefreshLogBox', () => { | ^", "stack": [ "[project]/index.js [ssr] (ecmascript) index.js (3:7)", - "", - "", - "", + "", + "", + "", ], } `) @@ -196,18 +196,18 @@ describe('ReactRefreshLogBox', () => { | ^", "stack": [ "eval index.js (3:7)", - "", - "", + "", + "", "eval ./pages/index.js", - "", - "", - "", - "", - "", - "", - "", - "", - "", + "", + "", + "", + "", + "", + "", + "", + "", + "", ], } `) @@ -284,8 +284,8 @@ describe('ReactRefreshLogBox', () => { "stack": [ "FunctionDefault FunctionDefault.js (1:51)", "Set.forEach (0:0)", - "", - "", + "${isTurbopack ? '' : ''}", + "${isTurbopack ? '' : ''}", ], } `) @@ -302,8 +302,8 @@ describe('ReactRefreshLogBox', () => { "stack": [ "FunctionDefault FunctionDefault.js (1:51)", "Set.forEach (0:0)", - "", - "", + "${isTurbopack ? '' : ''}", + "${isTurbopack ? '' : ''}", ], } `) @@ -413,7 +413,6 @@ describe('ReactRefreshLogBox', () => { } }) - // Module trace is only available with webpack 5 test('conversion to class component (1)', async () => { await using sandbox = await createSandbox(next) const { browser, session } = sandbox @@ -459,6 +458,8 @@ describe('ReactRefreshLogBox', () => { ` ) + // TODO(veil): ignore-list Webpack runtime (https://linear.app/vercel/issue/NDX-945) + // TODO(veil): Don't bail in Turbopack for sources outside of the project (https://linear.app/vercel/issue/NDX-944) if (isReact18) { await expect(browser).toDisplayRedbox(` { @@ -472,8 +473,8 @@ describe('ReactRefreshLogBox', () => { "stack": [ "ClickCount.render Child.js (4:11)", "Set.forEach (0:0)", - "", - "", + "${isTurbopack ? '' : ''}", + "${isTurbopack ? '' : ''}", ], } `) @@ -490,8 +491,8 @@ describe('ReactRefreshLogBox', () => { "stack": [ "ClickCount.render Child.js (4:11)", "Set.forEach (0:0)", - "", - "", + "${isTurbopack ? '' : ''}", + "${isTurbopack ? '' : ''}", ], } `) diff --git a/test/development/acceptance/error-recovery.test.ts b/test/development/acceptance/error-recovery.test.ts index 89eb366702767..5fa42335eb033 100644 --- a/test/development/acceptance/error-recovery.test.ts +++ b/test/development/acceptance/error-recovery.test.ts @@ -249,6 +249,8 @@ describe('pages/ error recovery', () => { ` ) + // TODO(veil): ignore-list Webpack runtime (https://linear.app/vercel/issue/NDX-945) + // TODO(veil): Don't bail in Turbopack for sources outside of the project (https://linear.app/vercel/issue/NDX-944) // Somehow we end up with two in React 18 due to React's attempt to recover from this error. if (isReact18) { await expect(browser).toDisplayRedbox(` @@ -263,8 +265,8 @@ describe('pages/ error recovery', () => { "stack": [ "Child child.js (3:9)", "Set.forEach (0:0)", - "", - "", + "${isTurbopack ? '' : ''}", + "${isTurbopack ? '' : ''}", ], } `) @@ -281,8 +283,8 @@ describe('pages/ error recovery', () => { "stack": [ "Child child.js (3:9)", "Set.forEach (0:0)", - "", - "", + "${isTurbopack ? '' : ''}", + "${isTurbopack ? '' : ''}", ], } `) @@ -527,6 +529,8 @@ describe('pages/ error recovery', () => { await expect(session.getRedboxSource()).resolves.toInclude('render() {') }) + // TODO(veil): ignore-list Webpack runtime (https://linear.app/vercel/issue/NDX-945) + // TODO(veil): Don't bail in Turbopack for sources outside of the project (https://linear.app/vercel/issue/NDX-944) // Somehow we end up with two in React 18 due to React's attempt to recover from this error. if (isReact18) { await expect(browser).toDisplayRedbox(` @@ -541,8 +545,8 @@ describe('pages/ error recovery', () => { "stack": [ "ClassDefault.render index.js (5:11)", "Set.forEach (0:0)", - "", - "", + "${isTurbopack ? '' : ''}", + "${isTurbopack ? '' : ''}", ], } `) @@ -564,22 +568,22 @@ describe('pages/ error recovery', () => { `) } else { await expect(browser).toDisplayRedbox(` - { - "count": 1, - "description": "Error: nooo", - "environmentLabel": null, - "label": "Unhandled Runtime Error", - "source": "index.js (5:11) @ ClassDefault.render - > 5 | throw new Error('nooo'); - | ^", - "stack": [ - "ClassDefault.render index.js (5:11)", - "Set.forEach (0:0)", - "", - "", - ], - } - `) + { + "count": 1, + "description": "Error: nooo", + "environmentLabel": null, + "label": "Unhandled Runtime Error", + "source": "index.js (5:11) @ ClassDefault.render + > 5 | throw new Error('nooo'); + | ^", + "stack": [ + "ClassDefault.render index.js (5:11)", + "Set.forEach (0:0)", + "${isTurbopack ? '' : ''}", + "${isTurbopack ? '' : ''}", + ], + } + `) } } }) @@ -627,6 +631,8 @@ describe('pages/ error recovery', () => { ` ) + // TODO(veil): ignore-list Webpack runtime (https://linear.app/vercel/issue/NDX-945) + // TODO(veil): Don't bail in Turbopack for sources outside of the project (https://linear.app/vercel/issue/NDX-944) // We get an error because Foo didn't import React. Fair. // Somehow we end up with two in React 18 due to React's attempt to recover from this error. if (isReact18) { @@ -642,8 +648,8 @@ describe('pages/ error recovery', () => { "stack": [ "Foo Foo.js (3:3)", "Set.forEach (0:0)", - "", - "", + "${isTurbopack ? '' : ''}", + "${isTurbopack ? '' : ''}", ], } `) @@ -660,8 +666,8 @@ describe('pages/ error recovery', () => { "stack": [ "Foo Foo.js (3:3)", "Set.forEach (0:0)", - "", - "", + "${isTurbopack ? '' : ''}", + "${isTurbopack ? '' : ''}", ], } `) diff --git a/test/development/pages-dir/client-navigation/index.test.ts b/test/development/pages-dir/client-navigation/index.test.ts index 02508fb601f48..981301d28a7f1 100644 --- a/test/development/pages-dir/client-navigation/index.test.ts +++ b/test/development/pages-dir/client-navigation/index.test.ts @@ -1482,10 +1482,10 @@ describe('Client Navigation', () => { | ^", "stack": [ "eval pages/error-in-the-browser-global-scope.js (2:9)", - "", - "", - "", - "", + "", + "", + "", + "", ], } `) diff --git a/test/e2e/app-dir/non-root-project-monorepo/non-root-project-monorepo.test.ts b/test/e2e/app-dir/non-root-project-monorepo/non-root-project-monorepo.test.ts index fce815a1587d7..8b93b42d174fd 100644 --- a/test/e2e/app-dir/non-root-project-monorepo/non-root-project-monorepo.test.ts +++ b/test/e2e/app-dir/non-root-project-monorepo/non-root-project-monorepo.test.ts @@ -171,10 +171,10 @@ describe('non-root-project-monorepo', () => { expect(normalizeStackTrace(await getRedboxCallStack(browser))) .toMatchInlineSnapshot(` "eval app/separate-file.ts (1:7) - - - - + + + + innerArrowFunction app/source-maps-ssr/page.tsx (16:3) innerFunction app/source-maps-ssr/page.tsx (12:3) Page app/source-maps-ssr/page.tsx (6:5)" @@ -216,10 +216,10 @@ describe('non-root-project-monorepo', () => { expect(normalizeStackTrace(await getRedboxCallStack(browser))) .toMatchInlineSnapshot(` "eval app/separate-file.ts (1:7) - - - - + + + + innerArrowFunction app/source-maps-client/page.tsx (17:3) innerFunction app/source-maps-client/page.tsx (13:3) effectCallback app/source-maps-client/page.tsx (7:5)"