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

Resolver should not swallow errors from the Relay Network Calls #111

Closed
jpbarela opened this issue Jan 26, 2018 · 21 comments
Closed

Resolver should not swallow errors from the Relay Network Calls #111

jpbarela opened this issue Jan 26, 2018 · 21 comments

Comments

@jpbarela
Copy link

The current resolver appears to swallow all of errors coming from the network layer leading to behavior that isn't similar to a standard found router or a relay-modern QueryRenderer. A developer can turn on error handling by explicitly including a render in each route but this seems error prone.

I seems like three possible solutions might make sense:

  1. Enable the resolver to receive an error component that is called if the GraphQL network layer returns an error similar to a QueryRenderer
  2. Throw the error to enable the FarceRouter to receive the error and let the developer work at that layer
  3. Enable the resolver to receive an optional render so that the resolver behavior more similarly to a standard found router.

I think there's some merit in each approach and I'd be happy to take the first crack at implementing any of these and submitting a PR or working on another solution if these don't seem right.

@taion
Copy link
Member

taion commented Jan 26, 2018

See facebook/relay#1913.

What's the difference with a normal <QueryRenderer>? The idea is that the render you pass into each route is roughly the same as the one you'd pass into <QueryRenderer>.

I believe the same error handling behavior holds in <QueryRenderer> as well – you need to explicitly handle error in render. Is that not the case?

@jwaldrip
Copy link

jwaldrip commented Jan 26, 2018

@taion Why not throw the error by default in found and allow the developer to implement error boundaries?

@jpbarela
Copy link
Author

I think I understand the idea on render but then how do you run a global render function?

I was thinking of the resolve as taking a similar position to a QueryRender at the root level of an application and then an individual render on the route as a nested QueryRender. If this is the case then there is no way to pass an error handler to the root QueryRender.

Is this the way its intended to be used or is the intent for each route to act as it's own QueryRenderer?

@taion
Copy link
Member

taion commented Jan 26, 2018

I don't throw the error by default here because I want to match the semantics have having each route act as its own <QueryRenderer>. The intent is in fact that each route behavior as if it were its own <QueryRenderer>.

@jpbarela
Copy link
Author

Ok, that makes sense. I might try to update the docs to help explain that concept.

Thanks for your quick response!

@taion
Copy link
Member

taion commented Jan 26, 2018

What docs? \:

You might want to add a note to #56.

@taion taion closed this as completed Jan 26, 2018
@dminkovsky
Copy link
Contributor

dminkovsky commented Feb 6, 2018

Following up with #86 (comment) (just found this issue after posting that comment):

The idea is that the render you pass into each route is roughly the same as the one you'd pass into <QueryRenderer>.

That makes perfect sense. Since <QueryRenderer> gives you access to the errors that resulted from the query, shouldn't the errors also be available in a route's render() or Component#render()?

@dminkovsky
Copy link
Contributor

dminkovsky commented Feb 6, 2018

Oh my bad, I guess that's a network error, and not the errors array from a result?

@taion
Copy link
Member

taion commented Feb 6, 2018

@dminkovsky Yes. I believe facebook/relay#1913 still stands. Right now I just throw in the network layer. It's not ideal but I don't need partial results quite yet. It's not that <QueryRenderer> doesn't ever give you errors; it's that the semantics of those errors are undesirable.

@taion
Copy link
Member

taion commented Feb 6, 2018

By "I just throw in the network layer", I mean this is what I do in my applications – not in this library.

@adeperio
Copy link

HI I am running into this issue at the moment. Testing for scenarios where network is down (as opposed to errors coming back from the graphql serve). Getting the network error back in the fetchQuery catch section. But is there a way to specify a render state for a network connectivity issue, rather than just not rendering the component? I've tried re-throwing network errors from the catch state but the component still doesn't render. I think this is a different problem from facebook/relay#1913? Is this a seperate issue to this one?

@taion
Copy link
Member

taion commented Feb 26, 2018

You maybe able to provide additional details on the error that you throw from there, then use that to control rendering.

@adeperio
Copy link

Hi thanks @taion. Any error I throw from the catch at the moment looks like it just logs to the console in the inspector. I can't seem to find where I can return an alternate render for error states.

Here is my current setup (with a custom wrapper that populates an access token wrapper in the fetch header):
image

I was expecting to see the error render state returned back as specified in here:...
image

But it doesn't really seem to be executing that renderError block? Is there someplace else I should be looking into?

@taion
Copy link
Member

taion commented Feb 27, 2018

If you want to hit the global renderError, then you need to throw an HttpError in render on your route. The error in the args to render has the same semantics as on <QueryRenderer>. You can choose to pass through whatever in the HttpError that you throw.

@adeperio
Copy link

Ahh ok, I think my problem might be before that.
Because its a network connectivity error (ie server is down), it doesn't even make it to the render method in my component. Is this something I handle elsewhere?

@adeperio
Copy link

PS I'd prefer not to hit the global error handler. At the moment I can't render anything on network connectivity issues so I'm trying there, and also my component. For reference my component looks like this:

image

The console log line with this.props.error never gets called. Do you know if there is a way for the component itself to get network connectivity feedback directly?

Thanks again @taion

@taion
Copy link
Member

taion commented Feb 27, 2018

You need render on the route. So like <Route render={...}>.

@adeperio
Copy link

Ahh ok. Like this?
image
image

@taion
Copy link
Member

taion commented Feb 27, 2018

Mostly, but don't use createRender there. It should be exactly like the render callback you pass into <QueryRenderer>.

@adeperio
Copy link

Awesome! Thanks @taion - that's resolved my issue :) If you wanted me to mention this in issue #56 as well just let me know. Also happy to look into adding some code for this in the examples so just tag me in an issue if you like.

Thanks again for your help!

Here's my final sample code for any one else it might help:
image

@taion
Copy link
Member

taion commented Feb 27, 2018

I added a note.

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

No branches or pull requests

5 participants