Design docs #923
Design docs #923
Conversation
| @@ -0,0 +1,29 @@ | |||
| # Error handling - design proposal | |||
There was a problem hiding this comment.
This file name is misspelled :]
|
@jbaxleyiii @tmeasday would be curious to get your thoughts about the error proposal. If you have feedback on design docs in general that would be useful, too. Anyone else's feedback is of course welcome and appreciated as well! |
|
It seems reasonable. What do we thing RA should do? There seem to be 3 cases
Do we need to distinguish between those cases? It seems challenging if we are trying to include the errors on the standard |
| - universally compatible | ||
| - easy to understand and use | ||
| - compare the proposed solution with obvious alternatives and show that | ||
| - the proposed solution is better than its alternatives with respect to the Apollo vision. |
There was a problem hiding this comment.
I'd also add some factors like:
- this is the simplest way to solve the proposed problem
- this should be in this package and not in a different package or a new one altogether
There was a problem hiding this comment.
I feel like your first point is included in the comparison with alternatives. I'll add the second point.
PS: I actually had a more complete list, but I felt like it was getting too long. I'd say let's start simple and add more stuff later if we see that it's necessary.
| # Error handling - design proposal | ||
|
|
||
| ## Motivation | ||
| ApolloClient currently does not deal with errors very gracefully. When the server returns any error, Apollo Client will discard the result and call `observer.error`. With the recent addition of error paths in the standard `formatError` function of graphql-js, we have an opportunity to improve error handling and write partial results to the store. |
There was a problem hiding this comment.
I'd call out "GraphQL errors" in particular. That is, errors that have a partial response and something in the errors field.
There was a problem hiding this comment.
I'd link to the PR with this addition, and the RFC on the spec: graphql/graphql-spec#230
|
|
||
| For errors of the first category, the server does not return any data that can be displayed to the user, only errors, so Apollo Client should call `observer.error` and not write data to the store. | ||
|
|
||
| For errors of the second category, the server returns data *and* errors, so there is data that can be displayed to the user and Apollo Client should call `observer.next` with `{ data, errors }`, and write as much of the result to the store as possible. Any `null` fields in `result.data` should not be written to the store if there is an error with a path corresponding to that field. |
There was a problem hiding this comment.
I'm worried that this will make error handling extremely cumbersome. Right now it's very easy to find out if something went wrong - you just look at the error.
To put it a different way - how will users of react-apollo get notified about the GraphQL error?
There was a problem hiding this comment.
I'm not sure how it would be any more complicated now? It's just the way the error gets delivered that's different, depending on the semantics of the error.
|
|
||
| For errors of the second category, the server returns data *and* errors, so there is data that can be displayed to the user and Apollo Client should call `observer.next` with `{ data, errors }`, and write as much of the result to the store as possible. Any `null` fields in `result.data` should not be written to the store if there is an error with a path corresponding to that field. | ||
|
|
||
| Because initially not all servers will have support for error paths, a configuration option is necessary to tell Apollo Client to use the current default behavior - i.e. call `observer.error` and discard all data. |
There was a problem hiding this comment.
I'm not sure this is the right approach. I feel like we should treat errors without path the same way we did before.
There was a problem hiding this comment.
The same way as before = discard the result?
|
@stubailo I don't think error handling will be more cumbersome if GraphQL errors get delivered with the data. If the server returns data and errors, shouldn't you be able to pass both of those to the UI at the same time? If you call the next callback and the error callback separately, how can you be sure that these are associated? And which one do you call first? Will both of them re-render the component? On second thought though, maybe we can avoid complexity in our code by just sending all errors through the errors callback and all data through the next callback. Is that what you're proposing? @tmeasday I think we don't need to distinguish between 1 and 2, because that would make things complicated. |
|
@tmeasday I would say for the sake of simplicity we don't make that data available at the moment. If there's an error and no data is returned, then that's what you get from Apollo Client. One can always try to repeat that same query with |
Initial stages of draft for proposed design docs process.