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

feat(core): accept a backtrace passed in the notice #1049

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

BethanyBerkowitz
Copy link
Contributor

Status

READY

Description

In working on #926, I found it would be helpful if the core package would accept a backtrace passed in, rather than exclusively either parse the notice.stack string into a backtrace array or create its own backtrace. The existing react-native package has custom code to make nice backtraces for native iOS and android errors, which are not formatted the same as javascript errors, so they wouldn't be parsed correctly from notice.stack.

Todos

  • Tests
  • Documentation

@BethanyBerkowitz
Copy link
Contributor Author

@subzero10 @joshuap Since this is part of the core package (eek!) I want to check with both of you that this seems safe to do. I don't think we'd generally want users to pass in a custom backtrace like this, it would really just be for our other packages to use. The current documentation doesn't indicate that you can or should pass in a backtrace, and I don't see a reason to change that.

Copy link
Member

@subzero10 subzero10 left a comment

Choose a reason for hiding this comment

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

I can't think of anything against this.
I think it's OK to accept customization at this level, since this is the core package.
Maybe an alternative would be to make the makeNotice() method abstract, and have each client come up with its implementation, but that's not a good idea at this point: all current clients would duplicate almost all of this logic (browser, node and now react-native).

Copy link
Member

@joshuap joshuap left a comment

Choose a reason for hiding this comment

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

Yeah, this seems fine to me.

We do expose notice.backtrace as a notice property already, and users can modify it in beforeNotify handlers (e.g. to optimize grouping). So it's not a far cry to allow the property to be passed in—although I don't think we should recommend it to end-users.

@BethanyBerkowitz BethanyBerkowitz merged commit 9c56a9f into master Mar 23, 2023
@BethanyBerkowitz BethanyBerkowitz deleted the BKB/accept-backtrace branch March 23, 2023 17:07
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.

3 participants