d2iq-archive / reactive-graphql

A GraphQL implementation based around RxJS, very well suited for client side only GraphQL usage
Apache License 2.0
57 stars 5 forks source link

Aggregate errors in results instead of throwing them #13

Open alexstrat opened 5 years ago

alexstrat commented 5 years ago

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.

DanielMSchmidt commented 5 years ago

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

error block on result

What do you think about this approach?

alexstrat commented 5 years ago

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.

DanielMSchmidt commented 5 years ago

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.