-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Feat: Allow node stackwalk in Electron renderer #3710
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
Conversation
|
I've also got a branch that uses |
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.
Would it maybe be possible to just bail out if fs is not available?
function tryToReadFile(filename, resolve): void {
try {
const { readFile } = require('fs');
// move previous logic here
} catch (_e) {
return null;
}
}Although totally valid, the implementation from this PR adds a lot of indirection which is kinda hard to follow.
This comment has been minimized.
This comment has been minimized.
Any usage of |
|
I've refactored this to be a bit better in that rather than passing around a ref to It would still probably be better to refactor the source reading out of So in public eventFromException(exception: any, hint?: EventHint): PromiseLike<Event> {
const event = eventFromException(this._options, exception, hint);
return addSourcesToEvent(event);
}And in public eventFromException(exception: any, hint?: EventHint): PromiseLike<Event> {
return eventFromException(this._options, exception, hint);
}Do you think the unit tests are up to supporting me through that kind of refactor? 🙂 |
|
Closing in favour of #3729 |
Related to: getsentry/sentry-electron#308
This PR:
@sentry/nodefunctionseventFromExceptionandeventFromMessageout of backend and into their own file much like in the browser SDK.@sentry/electronreadFileas a parameter in the parser functionsWhy is this funky
readFilestuff required?The Electron renderer may or may not have access to node functionality. If
nodeIntegrationis enabled, full node is available. IfnodeIntegrationis disabled and a bundler is being used, it will complain thatfscannot be found/is unavailable. With this PR we would just not passreadFilewhen the stack parser is called from the Electron renderer and no source files will be read.What are the alternatives?
@sentry/electrondepending onnodeIntegration(ie.initandinitWithNode)fstoexternalsNodeIntegrationsentry integration that "patches" in this functionality?