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

Respect sourcemap's ignore list when printing errors in the terminal #71908

Merged
Show file tree
Hide file tree
Changes from all 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
81 changes: 50 additions & 31 deletions packages/next/src/server/patch-error-inspect.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,24 @@
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'
import { parseStack } from '../client/components/react-dev-overlay/server/middleware'
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')

Expand Down Expand Up @@ -40,46 +53,44 @@ 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('<anonymous>') ||
// file.startsWith('eval')
)
return file.startsWith('node:')
}

function getSourcemappedFrameIfPossible(
frame: StackFrame,
sourcemapConsumers: Map<string, SyncSourceMapConsumer>
sourceMapCache: SourceMapCache
): {
stack: StackFrame
stack: IgnoreableStackFrame
// DEV only
code: string | null
} | null {
if (frame.file === null) {
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,
})
Expand All @@ -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
Expand All @@ -107,6 +122,7 @@ function getSourcemappedFrameIfPossible(
lineNumber: sourcePosition.line,
// TODO: c&p from async createOriginalStackFrame but why not frame.arguments?
arguments: [],
ignored,
}

const codeFrame =
Expand Down Expand Up @@ -138,7 +154,7 @@ function parseAndSourceMap(error: Error): string {
}

const unsourcemappedStack = parseStack(unparsedStack)
const sourcemapConsumers = new Map<string, SyncSourceMapConsumer>()
const sourceMapCache: SourceMapCache = new Map()

let sourceMappedStack = ''
let sourceFrameDEV: null | string = null
Expand All @@ -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)
}
}
}
}
Expand Down
10 changes: 2 additions & 8 deletions test/e2e/app-dir/server-source-maps/server-source-maps.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')

Expand All @@ -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 |'
)
Expand All @@ -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')

Expand All @@ -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 |'
)
Expand Down
Loading