From b830c73295d314c572618dfacf4523c29d16a9e1 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Fri, 25 Oct 2024 18:38:03 +0200 Subject: [PATCH] Revert "Sourcemap errors in terminal by default (#71444)" This reverts commit af6f6aa96c8524dade596622052e684e3ec98637. --- .../react-dev-overlay/server/shared.ts | 3 +- .../error-inspect.tsx | 3 - packages/next/src/server/node-environment.ts | 3 - .../next/src/server/patch-error-inspect.ts | 228 ------------------ .../app-action-size-limit-invalid.test.ts | 5 +- ...mic-io-errors.prospective-fallback.test.ts | 1 - .../prospective-fallback/next.config.js | 1 - .../fixtures/default/app/layout.js | 7 - .../default/app/rsc-error-log-cause/page.js | 12 - .../app/rsc-error-log-custom-name/page.js | 12 - .../default/app/rsc-error-log/page.js | 11 - .../fixtures/default/next.config.js | 10 - .../server-source-maps.test.ts | 34 --- 13 files changed, 3 insertions(+), 327 deletions(-) delete mode 100644 packages/next/src/server/node-environment-extensions/error-inspect.tsx delete mode 100644 packages/next/src/server/patch-error-inspect.ts delete mode 100644 test/e2e/app-dir/server-source-maps/fixtures/default/app/layout.js delete mode 100644 test/e2e/app-dir/server-source-maps/fixtures/default/app/rsc-error-log-cause/page.js delete mode 100644 test/e2e/app-dir/server-source-maps/fixtures/default/app/rsc-error-log-custom-name/page.js delete mode 100644 test/e2e/app-dir/server-source-maps/fixtures/default/app/rsc-error-log/page.js delete mode 100644 test/e2e/app-dir/server-source-maps/fixtures/default/next.config.js delete mode 100644 test/e2e/app-dir/server-source-maps/server-source-maps.test.ts diff --git a/packages/next/src/client/components/react-dev-overlay/server/shared.ts b/packages/next/src/client/components/react-dev-overlay/server/shared.ts index 4fede2333ea2c..3696d51b88ba2 100644 --- a/packages/next/src/client/components/react-dev-overlay/server/shared.ts +++ b/packages/next/src/client/components/react-dev-overlay/server/shared.ts @@ -50,7 +50,7 @@ export function findSourcePackage({ export function getOriginalCodeFrame( frame: StackFrame, source: string | null -): string | null { +): string | null | undefined { if (!source || isInternal(frame.file)) { return null } @@ -65,7 +65,6 @@ export function getOriginalCodeFrame( column: frame.column ?? 0, }, }, - // TODO: Only in TTY { forceColor: true } ) } diff --git a/packages/next/src/server/node-environment-extensions/error-inspect.tsx b/packages/next/src/server/node-environment-extensions/error-inspect.tsx deleted file mode 100644 index e086cbd003bd6..0000000000000 --- a/packages/next/src/server/node-environment-extensions/error-inspect.tsx +++ /dev/null @@ -1,3 +0,0 @@ -import { patchErrorInspect } from '../patch-error-inspect' - -patchErrorInspect() diff --git a/packages/next/src/server/node-environment.ts b/packages/next/src/server/node-environment.ts index 4eefc81a16dc7..83d97ac3a1571 100644 --- a/packages/next/src/server/node-environment.ts +++ b/packages/next/src/server/node-environment.ts @@ -1,9 +1,6 @@ // This file should be imported before any others. It sets up the environment // for later imports to work properly. -// Import before anything else so that unexpected errors in other extensions are properly formatted. -import './node-environment-extensions/error-inspect' - import './node-environment-baseline' import './node-environment-extensions/random' import './node-environment-extensions/date' diff --git a/packages/next/src/server/patch-error-inspect.ts b/packages/next/src/server/patch-error-inspect.ts deleted file mode 100644 index 6d85dd186cbb0..0000000000000 --- a/packages/next/src/server/patch-error-inspect.ts +++ /dev/null @@ -1,228 +0,0 @@ -import { findSourceMap } 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' - -// TODO: Implement for Edge runtime -const inspectSymbol = Symbol.for('nodejs.util.inspect.custom') - -function frameToString(frame: StackFrame): string { - let sourceLocation = frame.lineNumber !== null ? `:${frame.lineNumber}` : '' - if (frame.column !== null && sourceLocation !== '') { - sourceLocation += `:${frame.column}` - } - return frame.methodName - ? ` at ${frame.methodName} (${frame.file}${sourceLocation})` - : ` at ${frame.file}${frame.lineNumber}:${frame.column}` -} - -function computeErrorName(error: Error): string { - // TODO: Node.js seems to use a different algorithm - // class ReadonlyRequestCookiesError extends Error {}` would read `ReadonlyRequestCookiesError: [...]` - // in the stack i.e. seems like under certain conditions it favors the constructor name. - return error.name || 'Error' -} - -function prepareUnsourcemappedStackTrace( - error: Error, - structuredStackTrace: any[] -): string { - const name = computeErrorName(error) - const message = error.message || '' - let stack = name + ': ' + message - for (let i = 0; i < structuredStackTrace.length; i++) { - stack += '\n at ' + structuredStackTrace[i].toString() - } - return stack -} - -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') - ) -} - -function getSourcemappedFrameIfPossible( - frame: StackFrame, - sourcemapConsumers: Map -): { - stack: StackFrame - // 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) { - return null - } - sourcemap = new SyncSourceMapConsumer( - // @ts-expect-error -- Module.SourceMap['version'] is number but SyncSourceMapConsumer wants a string - moduleSourcemap.payload - ) - sourcemapConsumers.set(frame.file, sourcemap) - } - - const sourcePosition = sourcemap.originalPositionFor({ - column: frame.column ?? 0, - line: frame.lineNumber ?? 1, - }) - - if (sourcePosition.source === null) { - return null - } - - const sourceContent: string | null = - sourcemap.sourceContentFor( - sourcePosition.source, - /* returnNullOnMissing */ true - ) ?? null - - const originalFrame: StackFrame = { - methodName: - sourcePosition.name || - // default is not a valid identifier in JS so webpack uses a custom variable when it's an unnamed default export - // Resolve it back to `default` for the method name if the source position didn't have the method. - frame.methodName - ?.replace('__WEBPACK_DEFAULT_EXPORT__', 'default') - ?.replace('__webpack_exports__.', ''), - column: sourcePosition.column, - file: sourcePosition.source, - lineNumber: sourcePosition.line, - // TODO: c&p from async createOriginalStackFrame but why not frame.arguments? - arguments: [], - } - - const codeFrame = - process.env.NODE_ENV !== 'production' - ? getOriginalCodeFrame(originalFrame, sourceContent) - : null - - return { - stack: originalFrame, - code: codeFrame, - } -} - -function parseAndSourceMap(error: Error): string { - // We overwrote Error.prepareStackTrace earlier so error.stack is not sourcemapped. - let unparsedStack = String(error.stack) - // We could just read it from `error.stack`. - // This works around cases where a 3rd party `Error.prepareStackTrace` implementation - // doesn't implement the name computation correctly. - const errorName = computeErrorName(error) - - let idx = unparsedStack.indexOf('react-stack-bottom-frame') - if (idx !== -1) { - idx = unparsedStack.lastIndexOf('\n', idx) - } - if (idx !== -1) { - // Cut off everything after the bottom frame since it'll be React internals. - unparsedStack = unparsedStack.slice(0, idx) - } - - const unsourcemappedStack = parseStack(unparsedStack) - const sourcemapConsumers = new Map() - - let sourceMappedStack = '' - let sourceFrameDEV: null | string = null - for (const frame of unsourcemappedStack) { - if (frame.file === null) { - sourceMappedStack += '\n' + frameToString(frame) - } else if (!shouldIgnoreListByDefault(frame.file)) { - const sourcemappedFrame = getSourcemappedFrameIfPossible( - frame, - sourcemapConsumers - ) - - 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 = sourcemappedFrame.code - } - // TODO: Hide if ignore-listed but consider what happens if every frame is ignore listed. - sourceMappedStack += '\n' + frameToString(sourcemappedFrame.stack) - } - } - } - - return ( - errorName + - ': ' + - error.message + - sourceMappedStack + - (sourceFrameDEV !== null ? '\n' + sourceFrameDEV : '') - ) -} - -export function patchErrorInspect() { - Error.prepareStackTrace = prepareUnsourcemappedStackTrace - - // @ts-expect-error -- TODO upstream types - // eslint-disable-next-line no-extend-native -- We're not extending but overriding. - Error.prototype[inspectSymbol] = function ( - depth: number, - inspectOptions: util.InspectOptions, - inspect: typeof util.inspect - ): string { - // Create a new Error object with the source mapping applied and then use native - // Node.js formatting on the result. - const newError = - this.cause !== undefined - ? // Setting an undefined `cause` would print `[cause]: undefined` - new Error(this.message, { cause: this.cause }) - : new Error(this.message) - - // TODO: Ensure `class MyError extends Error {}` prints `MyError` as the name - newError.stack = parseAndSourceMap(this) - - for (const key in this) { - if (!Object.prototype.hasOwnProperty.call(newError, key)) { - // @ts-expect-error -- We're copying all enumerable properties. - // So they definitely exist on `this` and obviously have no type on `newError` (yet) - newError[key] = this[key] - } - } - - const originalCustomInspect = (newError as any)[inspectSymbol] - // Prevent infinite recursion. - // { customInspect: false } would result in `error.cause` not using our inspect. - Object.defineProperty(newError, inspectSymbol, { - value: undefined, - enumerable: false, - writable: true, - }) - try { - return inspect(newError, { - ...inspectOptions, - depth: - (inspectOptions.depth ?? - // Default in Node.js - 2) - depth, - }) - } finally { - ;(newError as any)[inspectSymbol] = originalCustomInspect - } - } -} diff --git a/test/e2e/app-dir/actions/app-action-size-limit-invalid.test.ts b/test/e2e/app-dir/actions/app-action-size-limit-invalid.test.ts index f3cfb6c442080..bf0699962bf25 100644 --- a/test/e2e/app-dir/actions/app-action-size-limit-invalid.test.ts +++ b/test/e2e/app-dir/actions/app-action-size-limit-invalid.test.ts @@ -1,6 +1,5 @@ import { nextTestSetup } from 'e2e-utils' import { retry } from 'next-test-utils' -import stripAnsi from 'strip-ansi' import { accountForOverhead } from './account-for-overhead' const CONFIG_ERROR = @@ -27,7 +26,7 @@ describe('app-dir action size limit invalid config', () => { beforeAll(() => { const onLog = (log: string) => { - logs.push(stripAnsi(log.trim())) + logs.push(log.trim()) } next.on('stdout', onLog) @@ -116,7 +115,7 @@ describe('app-dir action size limit invalid config', () => { await retry(() => { expect(logs).toContainEqual( - expect.stringContaining('Error: Body exceeded 1.5mb limit') + expect.stringContaining('[Error]: Body exceeded 1.5mb limit') ) expect(logs).toContainEqual( expect.stringContaining( diff --git a/test/e2e/app-dir/dynamic-io-errors/dynamic-io-errors.prospective-fallback.test.ts b/test/e2e/app-dir/dynamic-io-errors/dynamic-io-errors.prospective-fallback.test.ts index d9c59ec790450..ab021df70a7fb 100644 --- a/test/e2e/app-dir/dynamic-io-errors/dynamic-io-errors.prospective-fallback.test.ts +++ b/test/e2e/app-dir/dynamic-io-errors/dynamic-io-errors.prospective-fallback.test.ts @@ -27,7 +27,6 @@ describe(`Dynamic IO Prospective Fallback`, () => { // we expect the build to fail } - // TODO: Assert on component stack expect(next.cliOutput).toContain( 'Route "/blog/[slug]": A component accessed data, headers, params, searchParams, or a short-lived cache without a Suspense boundary nor a "use cache" above it.' ) diff --git a/test/e2e/app-dir/dynamic-io-errors/fixtures/prospective-fallback/next.config.js b/test/e2e/app-dir/dynamic-io-errors/fixtures/prospective-fallback/next.config.js index 195ad29be3c54..bb76fc99dc6b6 100644 --- a/test/e2e/app-dir/dynamic-io-errors/fixtures/prospective-fallback/next.config.js +++ b/test/e2e/app-dir/dynamic-io-errors/fixtures/prospective-fallback/next.config.js @@ -1,6 +1,5 @@ module.exports = { experimental: { dynamicIO: true, - serverSourceMaps: true, }, } diff --git a/test/e2e/app-dir/server-source-maps/fixtures/default/app/layout.js b/test/e2e/app-dir/server-source-maps/fixtures/default/app/layout.js deleted file mode 100644 index a3a86a5ca1e12..0000000000000 --- a/test/e2e/app-dir/server-source-maps/fixtures/default/app/layout.js +++ /dev/null @@ -1,7 +0,0 @@ -export default function Root({ children }) { - return ( - - {children} - - ) -} diff --git a/test/e2e/app-dir/server-source-maps/fixtures/default/app/rsc-error-log-cause/page.js b/test/e2e/app-dir/server-source-maps/fixtures/default/app/rsc-error-log-cause/page.js deleted file mode 100644 index e94ad0efd24a9..0000000000000 --- a/test/e2e/app-dir/server-source-maps/fixtures/default/app/rsc-error-log-cause/page.js +++ /dev/null @@ -1,12 +0,0 @@ -export const dynamic = 'force-dynamic' - -function logError(cause) { - const error = new Error('Boom', { cause }) - console.error(error) -} - -export default function Page() { - const error = new Error('Boom') - logError(error) - return null -} diff --git a/test/e2e/app-dir/server-source-maps/fixtures/default/app/rsc-error-log-custom-name/page.js b/test/e2e/app-dir/server-source-maps/fixtures/default/app/rsc-error-log-custom-name/page.js deleted file mode 100644 index 97d67f67ea79b..0000000000000 --- a/test/e2e/app-dir/server-source-maps/fixtures/default/app/rsc-error-log-custom-name/page.js +++ /dev/null @@ -1,12 +0,0 @@ -export const dynamic = 'force-dynamic' - -class UnnamedError extends Error {} -class NamedError extends Error { - name = 'MyError' -} - -export default function Page() { - console.error(new UnnamedError('Foo')) - console.error(new NamedError('Bar')) - return null -} diff --git a/test/e2e/app-dir/server-source-maps/fixtures/default/app/rsc-error-log/page.js b/test/e2e/app-dir/server-source-maps/fixtures/default/app/rsc-error-log/page.js deleted file mode 100644 index 935bfa6630cbd..0000000000000 --- a/test/e2e/app-dir/server-source-maps/fixtures/default/app/rsc-error-log/page.js +++ /dev/null @@ -1,11 +0,0 @@ -export const dynamic = 'force-dynamic' - -function logError() { - const error = new Error('Boom') - console.error(error) -} - -export default function Page() { - logError() - return null -} diff --git a/test/e2e/app-dir/server-source-maps/fixtures/default/next.config.js b/test/e2e/app-dir/server-source-maps/fixtures/default/next.config.js deleted file mode 100644 index 70e8c4e01e19b..0000000000000 --- a/test/e2e/app-dir/server-source-maps/fixtures/default/next.config.js +++ /dev/null @@ -1,10 +0,0 @@ -/** - * @type {import('next').NextConfig} - */ -const nextConfig = { - experimental: { - serverSourceMaps: true, - }, -} - -module.exports = nextConfig 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 deleted file mode 100644 index 4174e83860b43..0000000000000 --- a/test/e2e/app-dir/server-source-maps/server-source-maps.test.ts +++ /dev/null @@ -1,34 +0,0 @@ -import * as path from 'path' -import { nextTestSetup } from 'e2e-utils' - -describe('app-dir - server source maps', () => { - const { skipped, next, isNextDev } = nextTestSetup({ - files: path.join(__dirname, 'fixtures/default'), - // Deploy tests don't have access to runtime logs. - // Manually verify that the runtime logs match. - skipDeployment: true, - }) - - if (skipped) return - - it('logged errors have a sourcemapped stack with a codeframe', async () => { - // TODO: Write test once we run with `--enable-source-maps` when `experimental.serverSourceMaps` is set - }) - - it('logged errors have a sourcemapped `cause`', async () => { - // TODO: Write test once we run with `--enable-source-maps` when `experimental.serverSourceMaps` is set - }) - - it('logged errors preserve their name', async () => { - await next.render('/rsc-error-log-custom-name') - - expect(next.cliOutput).toContain( - // TODO: isNextDev ? 'UnnamedError: Foo' : '[Error]: Foo' - isNextDev ? 'Error: Foo' : 'Error: Foo' - ) - expect(next.cliOutput).toContain( - // TODO: isNextDev ? 'NamedError [MyError]: Bar' : '[MyError]: Bar' - isNextDev ? 'Error [MyError]: Bar' : 'Error [MyError]: Bar' - ) - }) -})