-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ref(node): Refactor node stack parsing to use common parser #4612
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
ref(node): Refactor node stack parsing to use common parser #4612
Conversation
…to feat/separate-source-reading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 quick comments
} | ||
|
||
const base = `${ | ||
(require && require.main && require.main.filename && dirname(require.main.filename)) || global.process.cwd() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use optional chaining in node land since we down-compile when we transpile from TS -> JS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to revert the optional chaining because it broke a webpack test somewhere.
I'm going to have to some back the require issue later!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, wonder what's going on. Is it the tests we have in https://github.com/getsentry/sentry-javascript/tree/master/packages/node/test/manual?
I think this is partly why we have utils like
sentry-javascript/packages/utils/src/node.ts
Lines 28 to 31 in 6809dd2
export function dynamicRequire(mod: any, request: string): any { | |
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | |
return mod.require(request); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was actually the nextjs webpack tests that started failing.
The issue with dynamicRequire
is that it's only designed to work with module.require
.
module.require
only gives you access to the require function. It does not include any other properties like require.main
. This is only available on require
which isn't even a regular global.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting, TIL about module.require
details.
🤦♂️ This is better
68c47fc
to
b7966ce
Compare
} | ||
|
||
const base = `${ | ||
(require && require.main && require.main.filename && dirname(require.main.filename)) || global.process.cwd() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, wonder what's going on. Is it the tests we have in https://github.com/getsentry/sentry-javascript/tree/master/packages/node/test/manual?
I think this is partly why we have utils like
sentry-javascript/packages/utils/src/node.ts
Lines 28 to 31 in 6809dd2
export function dynamicRequire(mod: any, request: string): any { | |
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | |
return mod.require(request); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think we are good to merge after the node parser name change, and fixing the rebase in the issue template!
} | ||
|
||
const base = `${ | ||
(require && require.main && require.main.filename && dirname(require.main.filename)) || global.process.cwd() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting, TIL about module.require
details.
5ec09d7
to
1379939
Compare
This PR:
StackFrame | undefined
@sentry/utils
StackFrame[]
are reversedSyncPromise
in the backend and removed promises from where they are not necessary.