Skip to content
This repository has been archived by the owner on Sep 14, 2020. It is now read-only.

fix(iOS): Ensure reports on disk prior to app termination #290

Merged
merged 9 commits into from
Nov 22, 2018

Conversation

kattrali
Copy link
Contributor

Goal

(Extends #287)

Ensure report delivery while holding the native JS execution thread for as little time as possible in the case of non-fatal errors being reported. Updated the integration test to make the stack trace ridiculous.

Tests

  • Tested against physical devices as well as in the integration suite

Linked issues

Closes #287

  • Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
  • Usage friction - is the proposed change in usage cumbersome or complicated?
  • Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
  • Concurrency concerns - if components are accessed asynchronously, what issues will arise
  • Thoroughness of added tests and any missing edge cases
  • Idiomatic use of the language

fractalwrench
fractalwrench previously approved these changes Nov 21, 2018
Copy link
Contributor

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

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

Left two minor comments. As long as the header file is updated, this lgtm.

RCT_EXPORT_METHOD(notify:(NSDictionary *)options) {
RCT_EXPORT_METHOD(notify:(NSDictionary *)options
resolve:(RCTPromiseResolveBlock)resolve
reject:(RCTPromiseRejectBlock)reject) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should update the declaration in the header file also


@ReactMethod
public void notifyBlocking(ReadableMap payload, boolean blocking, com.facebook.react.bridge.Callback callback) {
public void notify(ReadableMap payload, com.facebook.react.bridge.Promise promise) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could get away without the fully qualified class name here - it was necessary previously to disambiguate between the two Callback classes

@kattrali
Copy link
Contributor Author

Both nice catches, thanks @fractalwrench !

@kattrali kattrali merged commit bd088d2 into master Nov 22, 2018
@kattrali kattrali deleted the kattrali/ensure-ios-report-on-disk branch November 22, 2018 00:17
Jekiwijaya pushed a commit to Jekiwijaya/bugsnag-react-native that referenced this pull request Mar 11, 2019
## Goal

Caches cocoapods on CI, which should reduce the amount of time required for jobs to run. This is recommended on the [Travis CI docs](https://docs.travis-ci.com/user/caching/#CocoaPods).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants