Skip to content
This repository has been archived by the owner on Dec 6, 2019. It is now read-only.

Network related crashes #501

Merged
merged 14 commits into from
Apr 13, 2018
Merged

Network related crashes #501

merged 14 commits into from
Apr 13, 2018

Conversation

conrad-vanl
Copy link
Contributor

@conrad-vanl conrad-vanl commented Apr 13, 2018

This should prevent crashes from occurring due to network errors.

This was a HUGE doozy to figure out. Apollo does this interesting thing where if you don't "access" data.error in your component, it throws an error. Like...

MyComponent = ({ data }) => {
   if (data.error) { console.log('apollo is happy because I access data.error'); }
   ...
};

graphql(query)(MyComponent);

And if you don't do that, you get a RSOD / crash as an error is thrown. This seems sensible, although in our case, errors were being thrown even though we had a global error handler setup (apollo-link-error). After reading this thread - apollographql/react-apollo#604 (comment) it seems this behavior can be inconsistent and I tried all 5 solutions offered in that comment.

In the end, we needed a combination of making sure our HOCs were passing down error objects, updating the apollo-link-error syntax, and some selective rendering of an error message.

This PR can be tested by loading the app up, going to a few screens, then turning on airplane mode. For example - open the app, turn on airplane, then navigate to search. This triggers a crash on current app. Same with profile, giving, etc.

Creator

  • Build relevant tests if any apply
  • Ensure there are no new warnings
  • Upload an animated GIF showcasing the solution (if it applies)
  • Set a relevant reviewer

Reviewer

  • Run test and make sure all tests pass
  • Review function and form on iOS
  • Review function and form on Android
  • Review function and form on Web
  • Review new test logic

@conrad-vanl conrad-vanl added the ready for review PR is ready for review label Apr 13, 2018
@conrad-vanl
Copy link
Contributor Author

@burkeshartsis I'm going to go ahead and use admin privs to merge this so I can get a build out. I tested it a bunch, and while I don't think it catches every network error, it sure does catch most of the problems. Fixed the audio issue too. Feel free to look at this after the fact, but PLEASE enjoy your 👶 time!!

@conrad-vanl conrad-vanl merged commit 85a87c6 into alpha Apr 13, 2018
@conrad-vanl conrad-vanl deleted the network-related-crashes branch April 13, 2018 22:48
@conrad-vanl conrad-vanl mentioned this pull request Apr 16, 2018
9 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ready for review PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants