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

Allow serializing underlying NSError objects, closes #10506 #10507

Closed
wants to merge 7 commits into from

Conversation

neilsarkar
Copy link
Contributor

@neilsarkar neilsarkar commented Oct 22, 2016

Explain the motivation for making this change. What existing problem does the pull request solve?

See #10506. A native NSError with NSUnderlyingErrorKey set causes a JSON stringify error from the websocket dispatcher if remote debugging is enabled.

Test plan (required)

I'm not familiar with the react native testing framework. Happy to add a test for this if someone can point me to where this part of the codebase is exercised :)

I did some spot checks with nil user dictionaries and nil underlying errors here. The case that this solves is testable using https://github.com/superseriouscompany/react-native-error-repro, specifically:

NSError *underlyingError = [NSError errorWithDomain:@"underlyingDomain" code:421 userInfo:nil];
NSError *err = [NSError errorWithDomain:@"domain" code:68 userInfo:@{@"NSUnderlyingError": underlyingError}];

reject(@"foo", @"bar", err);

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @nicklockwood and @javache to be potential reviewers.

davidaurelio and others added 2 commits October 22, 2016 14:57
Reviewed By: bestander

Differential Revision: D4051237

fbshipit-source-id: ebe919d336b8e4f5d58ef12186026aac37cbc7f2
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 22, 2016
@lacker
Copy link
Contributor

lacker commented Oct 24, 2016

It's a little odd but the unit tests for this area of the code are in Examples/UIExplorer/UIExplorerUnitTests/ . It would be great to add one! - just a simple one that ensures this one use case works.

@neilsarkar
Copy link
Contributor Author

Thanks for pointing me in the right direction @lacker, added a unit test. Let me know what else is needed.

@lacker
Copy link
Contributor

lacker commented Oct 26, 2016

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Oct 26, 2016
@facebook-github-bot
Copy link
Contributor

@lacker has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Oct 28, 2016
Summary:
Explain the **motivation** for making this change. What existing problem does the pull request solve?

See facebook/react-native#10506. A native `NSError` with `NSUnderlyingErrorKey` set causes a JSON stringify error from the websocket dispatcher if remote debugging is enabled.

**Test plan (required)**

I'm not familiar with the react native testing framework. Happy to add a test for this if someone can point me to where this part of the codebase is exercised :)

I did some spot checks with nil user dictionaries and nil underlying errors here. The case that this solves is testable using https://github.com/superseriouscompany/react-native-error-repro, specifically:

```objective-c
NSError *underlyingError = [NSError errorWithDomain:@"underlyingDomain" code:421 userInfo:nil];
NSError *err = [NSError errorWithDomain:@"domain" code:68 userInfo:@{@"NSUnderlyingError": underlyingError}];

reject(@"foo", @"bar", err);
```
Closes facebook/react-native#10507

Differential Revision: D4080802

Pulled By: lacker

fbshipit-source-id: 93a41d9e9a710e406a6ccac214a5617271b4bede
DanielMSchmidt pushed a commit to DanielMSchmidt/react-native that referenced this pull request Jan 4, 2017
Summary:
Explain the **motivation** for making this change. What existing problem does the pull request solve?

See facebook#10506. A native `NSError` with `NSUnderlyingErrorKey` set causes a JSON stringify error from the websocket dispatcher if remote debugging is enabled.

**Test plan (required)**

I'm not familiar with the react native testing framework. Happy to add a test for this if someone can point me to where this part of the codebase is exercised :)

I did some spot checks with nil user dictionaries and nil underlying errors here. The case that this solves is testable using https://github.com/superseriouscompany/react-native-error-repro, specifically:

```objective-c
NSError *underlyingError = [NSError errorWithDomain:@"underlyingDomain" code:421 userInfo:nil];
NSError *err = [NSError errorWithDomain:@"domain" code:68 userInfo:@{@"NSUnderlyingError": underlyingError}];

reject(@"foo", @"bar", err);
```
Closes facebook#10507

Differential Revision: D4080802

Pulled By: lacker

fbshipit-source-id: 93a41d9e9a710e406a6ccac214a5617271b4bede
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants