From 219b512879222c9b05afa8f04718ed38ca7b1b0f Mon Sep 17 00:00:00 2001 From: eps1lon Date: Sun, 27 Oct 2024 15:10:27 +0100 Subject: [PATCH] Respect sourcemap's ignore list when printing errors in the terminal --- .../next/src/server/patch-error-inspect.ts | 81 ++++++++++++------- .../server-source-maps.test.ts | 10 +-- 2 files changed, 52 insertions(+), 39 deletions(-) diff --git a/packages/next/src/server/patch-error-inspect.ts b/packages/next/src/server/patch-error-inspect.ts index 17e74797453ea..900aa180ffbe1 100644 --- a/packages/next/src/server/patch-error-inspect.ts +++ b/packages/next/src/server/patch-error-inspect.ts @@ -1,4 +1,4 @@ -import { findSourceMap } from 'module' +import { findSourceMap, type SourceMapPayload } from 'module' import type * as util from 'util' import { SourceMapConsumer as SyncSourceMapConsumer } from 'next/dist/compiled/source-map' import type { StackFrame } from 'next/dist/compiled/stacktrace-parser' @@ -6,6 +6,19 @@ import { parseStack } from '../client/components/react-dev-overlay/server/middle import { getOriginalCodeFrame } from '../client/components/react-dev-overlay/server/shared' import { workUnitAsyncStorage } from './app-render/work-unit-async-storage.external' +interface ModernRawSourceMap extends SourceMapPayload { + ignoreList?: number[] +} + +interface IgnoreableStackFrame extends StackFrame { + ignored: boolean +} + +type SourceMapCache = Map< + string, + { map: SyncSourceMapConsumer; raw: ModernRawSourceMap } +> + // TODO: Implement for Edge runtime const inspectSymbol = Symbol.for('nodejs.util.inspect.custom') @@ -40,25 +53,14 @@ function prepareUnsourcemappedStackTrace( } function shouldIgnoreListByDefault(file: string): boolean { - return ( - // TODO: Solve via `ignoreList` instead. Tricky because node internals are not part of the sourcemap. - file.startsWith('node:') - // C&P from setup-dev-bundler - // TODO: Taken from setup-dev-bundler but these seem too broad - // file.includes('web/adapter') || - // file.includes('web/globals') || - // file.includes('sandbox/context') || - // TODO: Seems too aggressive? - // file.includes('') || - // file.startsWith('eval') - ) + return file.startsWith('node:') } function getSourcemappedFrameIfPossible( frame: StackFrame, - sourcemapConsumers: Map + sourceMapCache: SourceMapCache ): { - stack: StackFrame + stack: IgnoreableStackFrame // DEV only code: string | null } | null { @@ -66,20 +68,29 @@ function getSourcemappedFrameIfPossible( return null } - let sourcemap = sourcemapConsumers.get(frame.file) - if (sourcemap === undefined) { - const moduleSourcemap = findSourceMap(frame.file) - if (moduleSourcemap === undefined) { + const sourceMapCacheEntry = sourceMapCache.get(frame.file) + let sourceMap: SyncSourceMapConsumer + let rawSourceMap: ModernRawSourceMap + if (sourceMapCacheEntry === undefined) { + const moduleSourceMap = findSourceMap(frame.file) + if (moduleSourceMap === undefined) { return null } - sourcemap = new SyncSourceMapConsumer( + rawSourceMap = moduleSourceMap.payload + sourceMap = new SyncSourceMapConsumer( // @ts-expect-error -- Module.SourceMap['version'] is number but SyncSourceMapConsumer wants a string - moduleSourcemap.payload + rawSourceMap ) - sourcemapConsumers.set(frame.file, sourcemap) + sourceMapCache.set(frame.file, { + map: sourceMap, + raw: rawSourceMap, + }) + } else { + sourceMap = sourceMapCacheEntry.map + rawSourceMap = sourceMapCacheEntry.raw } - const sourcePosition = sourcemap.originalPositionFor({ + const sourcePosition = sourceMap.originalPositionFor({ column: frame.column ?? 0, line: frame.lineNumber ?? 1, }) @@ -89,12 +100,16 @@ function getSourcemappedFrameIfPossible( } const sourceContent: string | null = - sourcemap.sourceContentFor( + sourceMap.sourceContentFor( sourcePosition.source, /* returnNullOnMissing */ true ) ?? null - const originalFrame: StackFrame = { + // TODO: O(n^2). Consider moving `ignoreList` into a Set + const sourceIndex = rawSourceMap.sources.indexOf(sourcePosition.source) + const ignored = rawSourceMap.ignoreList?.includes(sourceIndex) ?? false + + const originalFrame: IgnoreableStackFrame = { methodName: sourcePosition.name || // default is not a valid identifier in JS so webpack uses a custom variable when it's an unnamed default export @@ -107,6 +122,7 @@ function getSourcemappedFrameIfPossible( lineNumber: sourcePosition.line, // TODO: c&p from async createOriginalStackFrame but why not frame.arguments? arguments: [], + ignored, } const codeFrame = @@ -138,7 +154,7 @@ function parseAndSourceMap(error: Error): string { } const unsourcemappedStack = parseStack(unparsedStack) - const sourcemapConsumers = new Map() + const sourceMapCache: SourceMapCache = new Map() let sourceMappedStack = '' let sourceFrameDEV: null | string = null @@ -148,22 +164,25 @@ function parseAndSourceMap(error: Error): string { } else if (!shouldIgnoreListByDefault(frame.file)) { const sourcemappedFrame = getSourcemappedFrameIfPossible( frame, - sourcemapConsumers + sourceMapCache ) if (sourcemappedFrame === null) { sourceMappedStack += '\n' + frameToString(frame) } else { - // TODO: Use the first frame that's not ignore-listed if ( process.env.NODE_ENV !== 'production' && sourcemappedFrame.code !== null && - sourceFrameDEV === null + sourceFrameDEV === null && + // TODO: Is this the right choice? + !sourcemappedFrame.stack.ignored ) { sourceFrameDEV = sourcemappedFrame.code } - // TODO: Hide if ignore-listed but consider what happens if every frame is ignore listed. - sourceMappedStack += '\n' + frameToString(sourcemappedFrame.stack) + if (!sourcemappedFrame.stack.ignored) { + // TODO: Consider what happens if every frame is ignore listed. + sourceMappedStack += '\n' + frameToString(sourcemappedFrame.stack) + } } } } diff --git a/test/e2e/app-dir/server-source-maps/server-source-maps.test.ts b/test/e2e/app-dir/server-source-maps/server-source-maps.test.ts index 93b66f9580211..45aba1155c338 100644 --- a/test/e2e/app-dir/server-source-maps/server-source-maps.test.ts +++ b/test/e2e/app-dir/server-source-maps/server-source-maps.test.ts @@ -147,7 +147,7 @@ describe('app-dir - server source maps', () => { // FIXME: Turbopack resolver bug // FIXME: Turbopack build? bugs taint the whole dev server ;(isTurbopack ? it.skip : it)( - 'stack frames are not ignore-listed in ssr', + 'stack frames are ignore-listed in ssr', async () => { await next.render('/ssr-error-log-ignore-listed') @@ -161,9 +161,6 @@ describe('app-dir - server source maps', () => { '\n at logError (webpack:///app/ssr-error-log-ignore-listed/page.js?[search]:5:16)' + // FIXME: Method name should be "Page" '\n at logError (webpack:///app/ssr-error-log-ignore-listed/page.js?[search]:10:12)' + - (process.env.NEXT_TEST_CI - ? '\n at run (webpack:///node_modules/[pnpm]/internal-pkg/index.js?[search]:2:0)' - : '\n at run (webpack://[fixture-root]/node_modules/[pnpm]/internal-pkg/index.js?[search]:2:0)') + '\n at Page (webpack:///app/ssr-error-log-ignore-listed/page.js?[search]:10:6)' + '\n 3 |' ) @@ -177,7 +174,7 @@ describe('app-dir - server source maps', () => { // FIXME: Turbopack resolver bug // FIXME: Turbopack build? bugs taint the whole dev server ;(isTurbopack ? it.skip : it)( - 'stack frames are not ignore-listed in rsc', + 'stack frames are ignore-listed in rsc', async () => { await next.render('/rsc-error-log-ignore-listed') @@ -191,9 +188,6 @@ describe('app-dir - server source maps', () => { '\n at logError (webpack:///app/rsc-error-log-ignore-listed/page.js?[search]:4:16)' + // FIXME: Method name should be "Page" '\n at logError (webpack:///app/rsc-error-log-ignore-listed/page.js?[search]:9:12)' + - (process.env.NEXT_TEST_CI - ? '\n at run (webpack:///node_modules/[pnpm]/internal-pkg/index.js?[search]:2:0)' - : '\n at run (webpack://[fixture-root]/node_modules/[pnpm]/internal-pkg/index.js?[search]:2:0)') + '\n at Page (webpack:///app/rsc-error-log-ignore-listed/page.js?[search]:9:6)' + '\n 2 |' )