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

fix(node): Correctly resolve module name #10001

Merged
merged 1 commit into from
Jan 2, 2024
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
41 changes: 25 additions & 16 deletions packages/node/src/module.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { posix, sep } from 'path';

const isWindowsPlatform = sep === '\\';
import { dirname } from '@sentry/utils';

/** normalizes Windows paths */
function normalizeWindowsPath(path: string): string {
Expand All @@ -9,52 +8,62 @@ 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 =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: This is not really related to this PR, but it would be nice to have a comment explaining what & why we actually do here - e.g. why use require.main.filename over global.process.cwd() etc. Feel free to ignore this, but as somebody with little to no historical context on this, this appears very magic to me 😅

Copy link
Collaborator Author

@timfish timfish Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea other than this has been like this for 11+ years from raven-node:
https://github.com/getsentry/raven-node/blame/7019cb30efda40704105b7b510bc428ec0c1ee01/lib/utils.js#L87

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a good point though which is worth considering as it will be important for #9072.

I guess the issue with process.cwd() is that it might give us the repository root rather than the runtime root. For example, the app might be run from process.cwd() + '/packages/my-app/dist/' so if we use cwd, paths (and therefore modules, will always start packages.my-app.dist.*.

dirname(require.main.filename) will only give us an accurate runtime root if the entry point is in the root. I suspect this is true the majority of the time.

In the Deno SDK, due to permissions we might not have have access to cwd() or the file system. To work around this, we parse a new Error().stack from init and pick the common root path from the stack trace.

function appRootFromErrorStack(error: Error): string | undefined {
// We know at the other end of the stack from here is the entry point that called 'init'
// We assume that this stacktrace will traverse the root of the app
const frames = createStackParser(nodeStackLineParser())(error.stack || '');
const paths = frames
// We're only interested in frames that are in_app with filenames
.filter(f => f.in_app && f.filename)
.map(
f =>
(f.filename as string)
.replace(/^[A-Z]:/, '') // remove Windows-style prefix
.replace(/\\/g, '/') // replace all `\` instances with `/`
.split('/')
.filter(seg => seg !== ''), // remove empty segments
) as string[][];
if (paths.length == 0) {
return undefined;
}
if (paths.length == 1) {
// Assume the single file is in the root
return dirname(paths[0].join('/'));
}
// Iterate over the paths and bail out when they no longer have a common root
let i = 0;
while (paths[0][i] && paths.every(w => w[i] === paths[0][i])) {
i++;
}
return paths[0].slice(0, i).join('/');
}

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,
normalizeWindowsPathSeparator: boolean = isWindowsPlatform,
basePath: string = getBasePath(),
isWindows: boolean = sep === '\\',
): string | undefined {
if (!filename) {
return;
}

const normalizedFilename = normalizeWindowsPathSeparator ? normalizeWindowsPath(filename) : filename;
const normalizedBase = isWindows ? normalizeWindowsPath(basePath) : basePath;
const normalizedFilename = isWindows ? normalizeWindowsPath(filename) : filename;

// eslint-disable-next-line prefer-const
let { root, dir, base: basename, ext } = posix.parse(normalizedFilename);

const base = (require && require.main && require.main.filename && dir) || global.process.cwd();

const normalizedBase = `${base}/`;

// It's specifically a module
let file = basename;
let { dir, base: file, ext } = posix.parse(normalizedFilename);

if (ext === '.js' || ext === '.mjs' || ext === '.cjs') {
file = file.slice(0, ext.length * -1);
}

if (!root && !dir) {
if (!dir) {
// No dirname whatsoever
dir = '.';
}

let n = dir.lastIndexOf('/node_modules/');
let n = dir.lastIndexOf('/node_modules');
if (n > -1) {
// /node_modules/ is 14 chars
return `${dir.slice(n + 14).replace(/\//g, '.')}:${file}`;
}

// 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;
}
56 changes: 27 additions & 29 deletions packages/node/test/module.test.ts
Original file line number Diff line number Diff line change
@@ -1,42 +1,40 @@
import { getModuleFromFilename } from '../src/module';

function withFilename(fn: () => void, filename: string) {
const prevFilename = require.main?.filename;
if (require.main?.filename) {
require.main.filename = filename;
}

try {
fn();
} finally {
if (require.main && prevFilename) {
require.main.filename = prevFilename;
}
}
}

describe('getModuleFromFilename', () => {
test('Windows', () => {
withFilename(() => {
expect(getModuleFromFilename('C:\\Users\\users\\Tim\\Desktop\\node_modules\\module.js', true)).toEqual('module');
}, 'C:\\Users\\Tim\\app.js');
expect(
getModuleFromFilename('C:\\Users\\Tim\\node_modules\\some-dep\\module.js', 'C:\\Users\\Tim\\', true),
).toEqual('some-dep:module');

expect(getModuleFromFilename('C:\\Users\\Tim\\some\\more\\feature.js', 'C:\\Users\\Tim\\', true)).toEqual(
'some.more:feature',
);
});

test('POSIX', () => {
withFilename(() => {
expect(getModuleFromFilename('/Users/users/Tim/Desktop/node_modules/module.js')).toEqual('module');
}, '/Users/Tim/app.js');
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');
});

test('.mjs', () => {
expect(getModuleFromFilename('/Users/Tim/node_modules/some-dep/module.mjs', '/Users/Tim/')).toEqual(
'some-dep:module',
);
});

test('POSIX .mjs', () => {
withFilename(() => {
expect(getModuleFromFilename('/Users/users/Tim/Desktop/node_modules/module.mjs')).toEqual('module');
}, '/Users/Tim/app.js');
test('.cjs', () => {
expect(getModuleFromFilename('/Users/Tim/node_modules/some-dep/module.cjs', '/Users/Tim/')).toEqual(
'some-dep:module',
);
});

test('POSIX .cjs', () => {
withFilename(() => {
expect(getModuleFromFilename('/Users/users/Tim/Desktop/node_modules/module.cjs')).toEqual('module');
}, '/Users/Tim/app.js');
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');
});
});