Ignore-list stack frames in node_modules even if not explicitly ignor…

…e-listed by their sourcemaps (#73689)

.eslintignore

@@ -45,6 +45,8 @@ test/development/basic/hmr/components/parse-error.js
 packages/next-swc/docs/assets/**/*
 test/lib/amp-validator-wasm.js
 test/production/pages-dir/production/fixture/amp-validator-wasm.js
+test/e2e/app-dir/server-source-maps/fixtures/default/internal-pkg/sourcemapped.js
+test/e2e/app-dir/server-source-maps/fixtures/default/external-pkg/sourcemapped.js
 test/e2e/async-modules/amp-validator-wasm.js
 test/development/next-lint-eslint-formatter-compact/**/*.js
 

.prettierignore

@@ -47,6 +47,8 @@ bench/nested-deps/components/**/*
 **/.tina/__generated__/**
 test/lib/amp-validator-wasm.js
 test/production/pages-dir/production/fixture/amp-validator-wasm.js
+test/e2e/app-dir/server-source-maps/fixtures/default/internal-pkg/sourcemapped.js
+test/e2e/app-dir/server-source-maps/fixtures/default/external-pkg/sourcemapped.js
 test/e2e/async-modules/amp-validator-wasm.js
 
 # turbopack crates

packages/next/src/server/patch-error-inspect.ts

@@ -108,8 +108,12 @@ function prepareUnsourcemappedStackTrace(
   return stack
 }
 
-function shouldIgnoreListByDefault(file: string): boolean {
-  return file.startsWith('node:')
+function shouldIgnoreListGeneratedFrame(file: string): boolean {
+  return file.startsWith('node:') || file.includes('node_modules')
+}
+
+function shouldIgnoreListOriginalFrame(file: string): boolean {
+  return file.includes('node_modules')
 }
 
 /**
@@ -143,18 +147,23 @@ function findApplicableSourceMapPayload(
   }
 }
 
+interface SourcemappableStackFrame extends StackFrame {
+  file: NonNullable<StackFrame['file']>
+}
+
+/**
+ * @param frame
+ * @param sourceMapCache
+ * @returns The original frame if not sourcemapped.
+ */
 function getSourcemappedFrameIfPossible(
-  frame: StackFrame,
+  frame: SourcemappableStackFrame,
   sourceMapCache: SourceMapCache
 ): {
   stack: IgnoreableStackFrame
   // DEV only
   code: string | null
-} | null {
-  if (frame.file === null) {
-    return null
-  }
-
+} {
   const sourceMapCacheEntry = sourceMapCache.get(frame.file)
   let sourceMapConsumer: SyncSourceMapConsumer
   let sourceMapPayload: ModernSourceMapPayload
@@ -170,7 +179,17 @@ function getSourcemappedFrameIfPossible(
       nativeFindSourceMap(sourceURL)?.payload ??
       bundlerFindSourceMapPayload(sourceURL)
     if (maybeSourceMapPayload === undefined) {
-      return null
+      return {
+        stack: {
+          arguments: frame.arguments,
+          column: frame.column,
+          file: frame.file,
+          lineNumber: frame.lineNumber,
+          methodName: frame.methodName,
+          ignored: shouldIgnoreListGeneratedFrame(frame.file),
+        },
+        code: null,
+      }
     }
     sourceMapPayload = maybeSourceMapPayload
     sourceMapConsumer = new SyncSourceMapConsumer(
@@ -192,7 +211,17 @@ function getSourcemappedFrameIfPossible(
   })
 
   if (sourcePosition.source === null) {
-    return null
+    return {
+      stack: {
+        arguments: frame.arguments,
+        column: frame.column,
+        file: frame.file,
+        lineNumber: frame.lineNumber,
+        methodName: frame.methodName,
+        ignored: shouldIgnoreListGeneratedFrame(frame.file),
+      },
+      code: null,
+    }
   }
 
   const sourceContent: string | null =
@@ -209,6 +238,14 @@ function getSourcemappedFrameIfPossible(
   let ignored = false
   if (applicableSourceMap === undefined) {
     console.error('No applicable source map found in sections for frame', frame)
+  } else if (shouldIgnoreListOriginalFrame(sourcePosition.source)) {
+    // Externals may be libraries that don't ship ignoreLists.
+    // This is really taking control away from libraries.
+    // They should still ship `ignoreList` so that attached debuggers ignore-list their frames.
+    // TODO: Maybe only ignore library sourcemaps if `ignoreList` is absent?
+    // Though keep in mind that Turbopack omits empty `ignoreList`.
+    // So if we establish this convention, we should communicate it to the ecosystem.
+    ignored = true
   } else {
     // TODO: O(n^2). Consider moving `ignoreList` into a Set
     const sourceIndex = applicableSourceMap.sources.indexOf(
@@ -271,34 +308,28 @@ function parseAndSourceMap(error: Error): string {
   for (const frame of unsourcemappedStack) {
     if (frame.file === null) {
       sourceMappedStack += '\n' + frameToString(frame)
-    } else if (!shouldIgnoreListByDefault(frame.file)) {
+    } else {
       const sourcemappedFrame = getSourcemappedFrameIfPossible(
-        frame,
+        // We narrowed this earlier by bailing if `frame.file` is null.
+        frame as SourcemappableStackFrame,
         sourceMapCache
       )
 
-      if (sourcemappedFrame === null) {
-        sourceMappedStack += '\n' + frameToString(frame)
-      } else {
-        if (
-          process.env.NODE_ENV !== 'production' &&
-          sourcemappedFrame.code !== null &&
-          sourceFrameDEV === null &&
-          // TODO: Is this the right choice?
-          !sourcemappedFrame.stack.ignored
-        ) {
-          sourceFrameDEV = sourcemappedFrame.code
-        }
-        if (!sourcemappedFrame.stack.ignored) {
-          // TODO: Consider what happens if every frame is ignore listed.
-          sourceMappedStack += '\n' + frameToString(sourcemappedFrame.stack)
-        } else if (showIgnoreListed) {
-          sourceMappedStack +=
-            '\n' + dim(frameToString(sourcemappedFrame.stack))
-        }
+      if (
+        process.env.NODE_ENV !== 'production' &&
+        sourcemappedFrame.code !== null &&
+        sourceFrameDEV === null &&
+        // TODO: Is this the right choice?
+        !sourcemappedFrame.stack.ignored
+      ) {
+        sourceFrameDEV = sourcemappedFrame.code
+      }
+      if (!sourcemappedFrame.stack.ignored) {
+        // TODO: Consider what happens if every frame is ignore listed.
+        sourceMappedStack += '\n' + frameToString(sourcemappedFrame.stack)
+      } else if (showIgnoreListed) {
+        sourceMappedStack += '\n' + dim(frameToString(sourcemappedFrame.stack))
       }
-    } else if (showIgnoreListed) {
-      sourceMappedStack += '\n' + dim(frameToString(frame))
     }
   }
 

test/development/middleware-errors/index.test.ts

@@ -42,26 +42,16 @@ describe('middleware - development errors', () => {
           ? '\n ⨯ Error: boom' +
               // TODO(veil): Sourcemap to original name i.e. "default"
               '\n    at __TURBOPACK__default__export__ (middleware.js:3:14)' +
-              '\n  1 |' +
-              '\n  2 |       export default function () {' +
-              "\n> 3 |         throw new Error('boom')" +
-              '\n    |              ^' +
-              '\n  4 |       }' +
-              '\n'
+              '\n  1 |'
           : '\n ⨯ Error: boom' +
               '\n    at default (middleware.js:3:14)' +
-              // TODO(veil): Should be ignore-listed
-              '\n    at eval (webpack'
+              '\n  1 |'
+      )
+      expect(stripAnsi(next.cliOutput)).toContain(
+        '' +
+          "\n> 3 |         throw new Error('boom')" +
+          '\n    |              ^'
       )
-      if (isTurbopack) {
-        // already asserted on codeframe earlier
-      } else {
-        expect(stripAnsi(next.cliOutput)).toContain(
-          '' +
-            "\n> 3 |         throw new Error('boom')" +
-            '\n    |              ^'
-        )
-      }
     })
 
     it('renders the error correctly and recovers', async () => {
@@ -104,28 +94,17 @@ describe('middleware - development errors', () => {
               '\n    at throwError (middleware.js:4:14)' +
               // TODO(veil): Sourcemap to original name i.e. "default"
               '\n    at __TURBOPACK__default__export__ (middleware.js:7:8)' +
-              "\n  2 |       import { NextResponse } from 'next/server'" +
-              '\n  3 |       async function throwError() {' +
-              "\n> 4 |         throw new Error('async boom!')" +
-              '\n    |              ^' +
-              '\n  5 |       }' +
-              '\n  6 |       export default function () {' +
-              '\n  7 |         throwError()'
+              "\n  2 |       import { NextResponse } from 'next/server'"
           : '\n ⨯ unhandledRejection:  Error: async boom!' +
               '\n    at throwError (middleware.js:4:14)' +
               '\n    at throwError (middleware.js:7:8)' +
-              // TODO(veil): Should be ignore-listed
-              '\n    at eval (webpack'
+              "\n  2 |       import { NextResponse } from 'next/server'"
+      )
+      expect(stripAnsi(next.cliOutput)).toContain(
+        '' +
+          "\n> 4 |         throw new Error('async boom!')" +
+          '\n    |              ^'
       )
-      if (isTurbopack) {
-        // already asserted on codeframe earlier
-      } else {
-        expect(stripAnsi(next.cliOutput)).toContain(
-          '' +
-            "\n> 4 |         throw new Error('async boom!')" +
-            '\n    |              ^'
-        )
-      }
     })
 
     it('does not render the error', async () => {
@@ -169,14 +148,15 @@ describe('middleware - development errors', () => {
           ? '\n ⨯ Error [ReferenceError]: test is not defined' +
               '\n    at eval (middleware.js:4:8)' +
               '\n    at <unknown> (middleware.js:4:8)' +
-              // TODO(veil): Should be ignore-listed
-              '\n    at fn (node_modules'
+              // TODO(veil): Should be sourcemapped
+              '\n    at __TURBOPACK__default__export__ ('
           : '\n ⨯ Error [ReferenceError]: test is not defined' +
               // TODO(veil): Redundant and not clickable
               '\n    at eval (file://webpack-internal:///(middleware)/./middleware.js)' +
               '\n    at eval (middleware.js:4:8)' +
-              // TODO(veil): Should be ignore-listed
-              '\n    at fn (node_modules'
+              // TODO(veil): Redundant
+              '\n    at eval (middleware.js:4:8)' +
+              "\n  2 |       import { NextResponse } from 'next/server'"
       )
       expect(stripAnsi(next.cliOutput)).toContain(
         isTurbopack
@@ -187,8 +167,7 @@ describe('middleware - development errors', () => {
           : "\n ⚠ DynamicCodeEvaluationWarning: Dynamic Code Evaluation (e. g. 'eval', 'new Function') not allowed in Edge Runtime" +
               '\nLearn More: https://nextjs.org/docs/messages/edge-dynamic-code-evaluation' +
               '\n    at eval (middleware.js:4:8)' +
-              // TODO(veil): Should be ignore-listed
-              '\n    at eval (webpack'
+              "\n  2 |       import { NextResponse } from 'next/server'"
       )
     })
 

test/e2e/app-dir/server-source-maps/fixtures/default/app/rsc-error-log-ignore-listed/page.js

@@ -1,5 +1,8 @@
 import { connection } from 'next/server'
-import { run } from 'internal-pkg'
+import { run as runInternal } from 'internal-pkg'
+import { run as runInternalSourceMapped } from 'internal-pkg/sourcemapped'
+import { run as runExternal } from 'external-pkg'
+import { run as runExternalSourceMapped } from 'external-pkg/sourcemapped'
 
 function logError() {
   const error = new Error('Boom')
@@ -9,6 +12,14 @@ function logError() {
 export default async function Page() {
   await connection()
 
-  run(() => logError())
+  runInternal(function runWithInternal() {
+    runInternalSourceMapped(function runWithInternalSourceMapped() {
+      runExternal(function runWithExternal() {
+        runExternalSourceMapped(function runWithExternalSourceMapped() {
+          logError()
+        })
+      })
+    })
+  })
   return null
 }

test/e2e/app-dir/server-source-maps/fixtures/default/app/ssr-error-log-ignore-listed/page.js

@@ -1,12 +1,23 @@
 'use client'
-import { run } from 'internal-pkg'
+import { run as runInternal } from 'internal-pkg'
+import { run as runInternalSourceMapped } from 'internal-pkg/sourcemapped'
+import { run as runExternal } from 'external-pkg'
+import { run as runExternalSourceMapped } from 'external-pkg/sourcemapped'
 
 function logError() {
   const error = new Error('Boom')
   console.error(error)
 }
 
 export default function Page() {
-  run(() => logError())
+  runInternal(function runWithInternal() {
+    runInternalSourceMapped(function runWithInternalSourceMapped() {
+      runExternal(function runWithExternal() {
+        runExternalSourceMapped(function runWithExternalSourceMapped() {
+          logError()
+        })
+      })
+    })
+  })
   return null
 }

test/e2e/app-dir/server-source-maps/fixtures/default/external-pkg/index.js

@@ -0,0 +1,3 @@
+export function run(fn) {
+  return fn()
+}

test/e2e/app-dir/server-source-maps/fixtures/default/external-pkg/package.json

@@ -0,0 +1,13 @@
+{
+  "name": "external-pkg",
+  "private": true,
+  "type": "module",
+  "exports": {
+    ".": {
+      "default": "./index.js"
+    },
+    "./sourcemapped": {
+      "default": "./sourcemapped.js"
+    }
+  }
+}

test/e2e/app-dir/server-source-maps/fixtures/default/external-pkg/sourcemapped.ts

@@ -0,0 +1,6 @@
+// Compile with pnpm tsc test/e2e/app-dir/server-source-maps/fixtures/default/external-pkg/sourcemapped.ts --sourceMap --target esnext
+// tsc compile errors can be ignored
+type Fn<T> = () => T
+export function run<T>(fn: Fn<T>): T {
+  return fn()
+}

test/e2e/app-dir/server-source-maps/fixtures/default/internal-pkg/package.json

@@ -5,6 +5,9 @@
   "exports": {
     ".": {
       "default": "./index.js"
+    },
+    "./sourcemapped": {
+      "default": "./sourcemapped.js"
     }
   }
 }

test/e2e/app-dir/server-source-maps/fixtures/default/internal-pkg/sourcemapped.ts

@@ -0,0 +1,6 @@
+// Compile with p tsc test/e2e/app-dir/server-source-maps/fixtures/default/internal-pkg/sourcemapped.ts --sourceMap --target esnext
+// tsc compile errors can be ignored
+type Fn<T> = () => T
+export function run<T>(fn: Fn<T>): T {
+  return fn()
+}

test/e2e/app-dir/server-source-maps/fixtures/default/next.config.js

@@ -6,6 +6,7 @@ const nextConfig = {
     dynamicIO: true,
     serverSourceMaps: true,
   },
+  serverExternalPackages: ['external-pkg'],
 }
 
 module.exports = nextConfig

test/e2e/app-dir/server-source-maps/fixtures/default/package.json

@@ -1,5 +1,6 @@
 {
   "dependencies": {
-    "internal-pkg": "file:./internal-pkg"
+    "internal-pkg": "file:./internal-pkg",
+    "external-pkg": "file:./external-pkg"
   }
 }