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

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Dec 31, 2023

Closes #10000

  • Caches the basePath so dirname(require.main.filename) only gets called once
  • Now uses full path for module name
  • Now correctly handles inside node_modules
  • Add more tests

@timfish timfish marked this pull request as ready for review January 1, 2024 13:57

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('/');
}

@mydea
Copy link
Member

mydea commented Jan 2, 2024

💯 01, nice!

@AbhiPrasad AbhiPrasad merged commit e8e6ab8 into getsentry:develop Jan 2, 2024
55 checks passed
@timfish timfish deleted the fix/node-module-name branch January 2, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node: StackFrame.module incorrectly parsed from filename
3 participants