From eff57fa07316a9c654ec2bdc1da352b9050833ad Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 9 Jan 2024 18:24:05 +0100 Subject: [PATCH] feat(node): Make `getModuleFromFilename` compatible with ESM (#10061) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This removes usage of `require.main.filename` to bring better ESM compatability. An extra bonus in that now `createGetModuleFromFilename` is ESM compatible, we can use it in the Anr worker when `appRootPath` is supplied (ie. Electron only for now). ### ⚠️ Fingerprinting This may change `module` fingerprinting for ESM since previously when `require.main.filename` was not available, `process.cwd()` was used. --- packages/node-experimental/src/index.ts | 2 + packages/node/src/index.ts | 9 ++- packages/node/src/integrations/anr/worker.ts | 5 +- packages/node/src/module.ts | 83 +++++++++----------- packages/node/src/sdk.ts | 4 +- packages/node/test/module.test.ts | 39 ++++----- 6 files changed, 68 insertions(+), 74 deletions(-) diff --git a/packages/node-experimental/src/index.ts b/packages/node-experimental/src/index.ts index f9d065de52db..d44fb10da9c1 100644 --- a/packages/node-experimental/src/index.ts +++ b/packages/node-experimental/src/index.ts @@ -52,7 +52,9 @@ export { extractRequestData, // eslint-disable-next-line deprecation/deprecation deepReadDirSync, + // eslint-disable-next-line deprecation/deprecation getModuleFromFilename, + createGetModuleFromFilename, close, createTransport, // eslint-disable-next-line deprecation/deprecation diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index ba0f2333df6d..21735a64d6a1 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -86,7 +86,14 @@ export { defaultIntegrations, init, defaultStackParser, getSentryRelease } from export { addRequestDataToEvent, DEFAULT_USER_INCLUDES, extractRequestData } from '@sentry/utils'; // eslint-disable-next-line deprecation/deprecation export { deepReadDirSync } from './utils'; -export { getModuleFromFilename } from './module'; + +import { createGetModuleFromFilename } from './module'; +/** + * @deprecated use `createGetModuleFromFilename` instead. + */ +export const getModuleFromFilename = createGetModuleFromFilename(); +export { createGetModuleFromFilename }; + // eslint-disable-next-line deprecation/deprecation export { enableAnrDetection } from './integrations/anr/legacy'; diff --git a/packages/node/src/integrations/anr/worker.ts b/packages/node/src/integrations/anr/worker.ts index f87ff8bb672f..4f9278b5b78f 100644 --- a/packages/node/src/integrations/anr/worker.ts +++ b/packages/node/src/integrations/anr/worker.ts @@ -15,6 +15,8 @@ import { } from '@sentry/utils'; import { Session as InspectorSession } from 'inspector'; import { parentPort, workerData } from 'worker_threads'; + +import { createGetModuleFromFilename } from '../../module'; import { makeNodeTransport } from '../../transports'; import type { WorkerStartData } from './common'; @@ -158,8 +160,9 @@ if (options.captureStackTrace) { // copy the frames const callFrames = [...event.params.callFrames]; + const getModuleName = options.appRootPath ? createGetModuleFromFilename(options.appRootPath) : () => undefined; const stackFrames = callFrames.map(frame => - callFrameToStackFrame(frame, scripts.get(frame.location.scriptId), () => undefined), + callFrameToStackFrame(frame, scripts.get(frame.location.scriptId), getModuleName), ); // Evaluate a script in the currently paused context diff --git a/packages/node/src/module.ts b/packages/node/src/module.ts index 73364493ddb2..a2e93fe99115 100644 --- a/packages/node/src/module.ts +++ b/packages/node/src/module.ts @@ -8,62 +8,51 @@ function normalizeWindowsPath(path: string): string { .replace(/\\/g, '/'); // replace all `\` instances with `/` } -// We cache this so we don't have to recompute it -let basePath: string | undefined; - -function getBasePath(): string { - if (!basePath) { - const baseDir = - require && require.main && require.main.filename ? dirname(require.main.filename) : global.process.cwd(); - basePath = `${baseDir}/`; - } - - return basePath; -} - -/** Gets the module from a filename */ -export function getModuleFromFilename( - filename: string | undefined, - basePath: string = getBasePath(), +/** Creates a function that gets the module name from a filename */ +export function createGetModuleFromFilename( + basePath: string = dirname(process.argv[1]), isWindows: boolean = sep === '\\', -): string | undefined { - if (!filename) { - return; - } - +): (filename: string | undefined) => string | undefined { const normalizedBase = isWindows ? normalizeWindowsPath(basePath) : basePath; - const normalizedFilename = isWindows ? normalizeWindowsPath(filename) : filename; - // eslint-disable-next-line prefer-const - let { dir, base: file, ext } = posix.parse(normalizedFilename); + return (filename: string | undefined) => { + if (!filename) { + return; + } - if (ext === '.js' || ext === '.mjs' || ext === '.cjs') { - file = file.slice(0, ext.length * -1); - } + const normalizedFilename = isWindows ? normalizeWindowsPath(filename) : filename; - if (!dir) { - // No dirname whatsoever - dir = '.'; - } + // eslint-disable-next-line prefer-const + let { dir, base: file, ext } = posix.parse(normalizedFilename); - let n = dir.lastIndexOf('/node_modules'); - if (n > -1) { - return `${dir.slice(n + 14).replace(/\//g, '.')}:${file}`; - } + if (ext === '.js' || ext === '.mjs' || ext === '.cjs') { + file = file.slice(0, ext.length * -1); + } - // Let's see if it's a part of the main module - // To be a part of main module, it has to share the same base - n = `${dir}/`.lastIndexOf(normalizedBase, 0); - if (n === 0) { - let moduleName = dir.slice(normalizedBase.length).replace(/\//g, '.'); + if (!dir) { + // No dirname whatsoever + dir = '.'; + } - if (moduleName) { - moduleName += ':'; + let n = dir.lastIndexOf('/node_modules'); + if (n > -1) { + return `${dir.slice(n + 14).replace(/\//g, '.')}:${file}`; } - moduleName += file; - return moduleName; - } + // Let's see if it's a part of the main module + // To be a part of main module, it has to share the same base + n = `${dir}/`.lastIndexOf(normalizedBase, 0); + if (n === 0) { + let moduleName = dir.slice(normalizedBase.length).replace(/\//g, '.'); + + if (moduleName) { + moduleName += ':'; + } + moduleName += file; + + return moduleName; + } - return file; + return file; + }; } diff --git a/packages/node/src/sdk.ts b/packages/node/src/sdk.ts index 88a6b1a9074c..59a895b0809c 100644 --- a/packages/node/src/sdk.ts +++ b/packages/node/src/sdk.ts @@ -34,7 +34,7 @@ import { Spotlight, Undici, } from './integrations'; -import { getModuleFromFilename } from './module'; +import { createGetModuleFromFilename } from './module'; import { makeNodeTransport } from './transports'; import type { NodeClientOptions, NodeOptions } from './types'; @@ -240,7 +240,7 @@ export function getSentryRelease(fallback?: string): string | undefined { } /** Node.js stack parser */ -export const defaultStackParser: StackParser = createStackParser(nodeStackLineParser(getModuleFromFilename)); +export const defaultStackParser: StackParser = createStackParser(nodeStackLineParser(createGetModuleFromFilename())); /** * Enable automatic Session Tracking for the node process. diff --git a/packages/node/test/module.test.ts b/packages/node/test/module.test.ts index 04a6a95a7888..3fdcdccfa6eb 100644 --- a/packages/node/test/module.test.ts +++ b/packages/node/test/module.test.ts @@ -1,40 +1,33 @@ -import { getModuleFromFilename } from '../src/module'; +import { createGetModuleFromFilename } from '../src/module'; -describe('getModuleFromFilename', () => { - test('Windows', () => { - expect( - getModuleFromFilename('C:\\Users\\Tim\\node_modules\\some-dep\\module.js', 'C:\\Users\\Tim\\', true), - ).toEqual('some-dep:module'); +const getModuleFromFilenameWindows = createGetModuleFromFilename('C:\\Users\\Tim\\', true); +const getModuleFromFilenamePosix = createGetModuleFromFilename('/Users/Tim/'); - expect(getModuleFromFilename('C:\\Users\\Tim\\some\\more\\feature.js', 'C:\\Users\\Tim\\', true)).toEqual( - 'some.more:feature', +describe('createGetModuleFromFilename', () => { + test('Windows', () => { + expect(getModuleFromFilenameWindows('C:\\Users\\Tim\\node_modules\\some-dep\\module.js')).toEqual( + 'some-dep:module', ); + expect(getModuleFromFilenameWindows('C:\\Users\\Tim\\some\\more\\feature.js')).toEqual('some.more:feature'); }); test('POSIX', () => { - expect(getModuleFromFilename('/Users/Tim/node_modules/some-dep/module.js', '/Users/Tim/')).toEqual( - 'some-dep:module', - ); - - expect(getModuleFromFilename('/Users/Tim/some/more/feature.js', '/Users/Tim/')).toEqual('some.more:feature'); - expect(getModuleFromFilename('/Users/Tim/main.js', '/Users/Tim/')).toEqual('main'); + expect(getModuleFromFilenamePosix('/Users/Tim/node_modules/some-dep/module.js')).toEqual('some-dep:module'); + expect(getModuleFromFilenamePosix('/Users/Tim/some/more/feature.js')).toEqual('some.more:feature'); + expect(getModuleFromFilenamePosix('/Users/Tim/main.js')).toEqual('main'); }); test('.mjs', () => { - expect(getModuleFromFilename('/Users/Tim/node_modules/some-dep/module.mjs', '/Users/Tim/')).toEqual( - 'some-dep:module', - ); + expect(getModuleFromFilenamePosix('/Users/Tim/node_modules/some-dep/module.mjs')).toEqual('some-dep:module'); }); test('.cjs', () => { - expect(getModuleFromFilename('/Users/Tim/node_modules/some-dep/module.cjs', '/Users/Tim/')).toEqual( - 'some-dep:module', - ); + expect(getModuleFromFilenamePosix('/Users/Tim/node_modules/some-dep/module.cjs')).toEqual('some-dep:module'); }); test('node internal', () => { - expect(getModuleFromFilename('node.js', '/Users/Tim/')).toEqual('node'); - expect(getModuleFromFilename('node:internal/process/task_queues', '/Users/Tim/')).toEqual('task_queues'); - expect(getModuleFromFilename('node:internal/timers', '/Users/Tim/')).toEqual('timers'); + expect(getModuleFromFilenamePosix('node.js')).toEqual('node'); + expect(getModuleFromFilenamePosix('node:internal/process/task_queues')).toEqual('task_queues'); + expect(getModuleFromFilenamePosix('node:internal/timers')).toEqual('timers'); }); });