Skip to content
This repository has been archived by the owner on Oct 23, 2024. It is now read-only.

Aggregate errors in results instead of throwing them #13

Open
alexstrat opened this issue Jan 23, 2019 · 3 comments
Open

Aggregate errors in results instead of throwing them #13

alexstrat opened this issue Jan 23, 2019 · 3 comments

Comments

@alexstrat
Copy link
Contributor

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
Copy link
Contributor

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?

@alexstrat
Copy link
Contributor Author

alexstrat commented Jan 23, 2019

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
Copy link
Contributor

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 pushed a commit to dcos/dcos-ui that referenced this issue Mar 14, 2019
Reactive-GraphQL is currently throwing an error, completing the observable
In the future (d2iq-archive/reactive-graphql#13) we
would want to aggregate these non-structural errors in the output instead,
this is why I choose to deferr this test a little.
DanielMSchmidt pushed a commit to dcos/dcos-ui that referenced this issue Mar 15, 2019
Reactive-GraphQL is currently throwing an error, completing the observable
In the future (d2iq-archive/reactive-graphql#13) we
would want to aggregate these non-structural errors in the output instead,
this is why I choose to deferr this test a little.
DanielMSchmidt pushed a commit to dcos/dcos-ui that referenced this issue Mar 15, 2019
Reactive-GraphQL is currently throwing an error, completing the observable
In the future (d2iq-archive/reactive-graphql#13) we
would want to aggregate these non-structural errors in the output instead,
this is why I choose to deferr this test a little.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants