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

ios: bugfix for wrong userInfo #68

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

fatfatson
Copy link

bugfix for wrong userInfo

bugfix for wrong userInfo
@fatfatson fatfatson changed the title Update ReactNativeExceptionHandler.m ios: bugfix for wrong userInfo Jun 21, 2018
Copy link
Owner

@a7ul a7ul left a comment

Choose a reason for hiding this comment

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

Other than the changes mentioned. it looks good to me.

@@ -59,8 +81,9 @@ - (dispatch_queue_t)methodQueue
// ====================================

RCT_EXPORT_MODULE();

// METHOD TO INITIALIZE THE EXCEPTION HANDLER AND SET THE JS CALLBACK BLOCK
RCT_EXPORT_METHOD(raiseTestNativeError) { NSLog(@"RAISING A TEST EXCEPTION"); [NSException raise:@"TEST EXCEPTION" format:@"THIS IS A TEST EXCEPTION"]; }
Copy link
Owner

Choose a reason for hiding this comment

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

As mentioned on the issue before . I dont think we should add raiseTestNative Error as part of this module. Could you please remove these.


long letMeCrash(){
Copy link
Owner

Choose a reason for hiding this comment

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

Also please remove these test functions as well.

Copy link
Author

Choose a reason for hiding this comment

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

ok, I have opened a new PR.
I had thought that only the commits before opening are visible, so I commit my own modify after open it, but it's not the truth. now I make a special branch for this, hope it's ok this time~

Copy link
Author

Choose a reason for hiding this comment

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

by the way , except those test functions, getting and saving the slide is useful for symbol address translation, would you like it to be merged?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes. I want to merge everything except the test functions

Copy link
Owner

Choose a reason for hiding this comment

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

Please remove only the test functions.

@a7ul
Copy link
Owner

a7ul commented Jun 21, 2018

@fatfatson Can you include all the changes in one pr and close the rest ?

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.

2 participants