reactive-graphql icon indicating copy to clipboard operation
reactive-graphql copied to clipboard

Aggregate errors in results instead of throwing them

Open alexstrat opened this issue 6 years ago • 3 comments

According to GrapqhQL specification, any error (missing field, resolver raises..) should be caught, formatted (GraphQLError type in graphql-js) and added to the "errors" list in the response.

As of today, they are thrown in the observable error channel.

alexstrat avatar Jan 23 '19 20:01 alexstrat

Hmm, I think we need to distinguish here between errors that a user needs to handle and errors that are viable to work with. I would think that making everything a graphql type error would not leverage the paradigms rxjs helps us with.

Throw Observable

  • Could not parse query
  • Error in the implementation itself (ups)
  • Usage of things we did not implement

error block on result

  • Field resolver threw
  • Validation (not there yet) failed, so array returned where non is expected, e.g.

What do you think about this approach?

DanielMSchmidt avatar Jan 23 '19 20:01 DanielMSchmidt

Your suggestions:

  1. When query could not be parsed => throw on observable
  2. On error in the implementation itself (ups) => throw on observable
  3. Usage of things we did not implement => throw on observable
  4. Field resolver threw => push error in errors field of the result
  5. Validation (not there yet) failed, so array returned where non is expected, e.g. => push error in errors field of the result

100% agree with 4 and 5, and they are the most important! I think in 4, we also consider resolvers that return an Observable that end up throwing, right? myField: () => throwError(new Error())

The rest being more anecdotic and/or not clearly specified, so I don't have hard thoughts.

For 1, as discussed, for the reference, the specification is not clear and in the reference implementation, they decided to add any SyntaxError to the list of errors and to resolve normally the result with the errors key.

alexstrat avatar Jan 23 '19 21:01 alexstrat

I think in 4, we also consider resolvers that return an Observable that end up throwing, right?

Now we do 👍

I think this interface will have a nice usability, let's see when I have time to implement it, but it's my top priority on this project.

DanielMSchmidt avatar Jan 25 '19 12:01 DanielMSchmidt