Skip to content

Commit

Permalink
Respect sourcemap's ignore list when printing errors in the terminal (#…
Browse files Browse the repository at this point in the history 
…71908)

The terminal now displays the same stack as the default in Chrome DevTools. 

There's currently no affordance to show ignore-listed stacks. I don't think we commonly need to show ignore-listed frames. If this is required, a Node.js debugger can be attached where ignore-listed frames can be show
  • Loading branch information
@eps1lon
eps1lon authored Oct 29, 2024
1 parent 97153cc commit 8403780
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 39 deletions.
81 changes: 50 additions & 31 deletions packages/next/src/server/patch-error-inspect.ts
View file
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
View file
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

0 comments on commit 8403780

Please sign in to comment.