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

[react-native] capture and report fatals on next launch #626

Merged
merged 1 commit into from
Aug 2, 2016
Merged

[react-native] capture and report fatals on next launch #626

merged 1 commit into from
Aug 2, 2016

Conversation

nevir
Copy link
Contributor

@nevir nevir commented Jun 24, 2016

Addresses #468, #533, and maybe #489


When a fatal (global) error is thrown, raven-js now captures it and persists it before calling the original error handler when in production mode (e.g. so the JS error can bubble out and crash the app). Then, when the react native plugin initializes, it checks for a persisted error, and reports it.

Some finer details:

  • In development mode, errors are not persisted, and the default error handler is always called. This allows redboxes to pop as expected.
  • Any subsequent fatals thrown while raven-js is persisting the first one are ignored (we want to crash ASAP).
  • If the error fails to report; it won't be cleared. This provides a very rudimentary retry mechanism (true offline support should be implemented more robustly, IMO).
  • I snuck in Raven.addShouldSendCallback so that the react-native plugin doesn't bash over existing configurations
  • onInitialize is useful for detecting if an error was previously reported (say, if you want to pop a "we're sorry" alert, or fix up your store)

This change is Reviewable

@Sija
Copy link
Contributor

Sija commented Jun 24, 2016

This would be handy as a transport in other environments too (think no internet situations)

* @return {Raven}
*/
addShouldSendCallback: function(callback) {
var prevCallback = this._globalOptions.shouldSendCallback;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sigh, again hesitant to add more API methods – even though I know this solves some serious problems.

We've talked about just merging shouldSendCallback / dataCallback in a major version change (4.0).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm writing a small plugin to solve #279. This API method would be useful.

@SergeyKorochansky
Copy link

I also bumped into this issue. Thank you for PR!

@SergeyKorochansky
Copy link

@nevir Thanks for PR. I merged your PR in our fork and it works great!

@benvinegar
Copy link
Contributor

@nevir – possible to update this to reflect changes in #636?

@Kerumen
Copy link

Kerumen commented Jul 26, 2016

Ping @nevir, it would be awesome if you could update the PR. Thanks!

@nevir
Copy link
Contributor Author

nevir commented Jul 26, 2016

Ack, sorry! Will get to this in the new few days

@nevir
Copy link
Contributor Author

nevir commented Jul 31, 2016

Alright, rebased it off of the latest! PTAL

@Kerumen
Copy link

Kerumen commented Aug 2, 2016

Ping @benvinegar

@benvinegar benvinegar merged commit d9c9d85 into getsentry:master Aug 2, 2016
@benvinegar
Copy link
Contributor

Here goes nothing!

@nevir
Copy link
Contributor Author

nevir commented Aug 2, 2016

Sweet!

@abc123s
Copy link

abc123s commented Aug 12, 2016

@nevir
I have a question on the changes in the this commit.

  1. In lines 87-104 of plugins/react-native.js, isn't it possible for originalCallback to be undefined, causing the plugin to crash for non-fatal errors?

@blackxored
Copy link

@abc123s Indeed, I'm actually having that exact error atm.

@TSMMark
Copy link

TSMMark commented Dec 16, 2016

For the record, #692 addresses @abc123s's issue

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.

9 participants