Better error handling

[jasonkuhrt]

Perceived Problem

• Error handling is confusing
• There is an error policy configuration that allows errors to be returned or thrown
• That configuration has the concept of ignore which is confusing
• When reading the code, it is not clear what will happen
• When interacting with the types, it is not clear what will happen b/c TS implementation lacks type mapping that reflects the current config rawRequest() return type wrong: can never include errors #174 (comment)
• HTTP errors are always thrown, only GraphQL errors part of the above system
• input validation will also lead to throws. For example abc as a url leads to a thrown fetch error of TypeError: Only absolute URLs are supported.

Ideas / Proposed Solution(s)

• Remove error policy configuration

• Make OrThrow function variants

• Make current functions default to not throwing (BREAKING CHANGE)

   request()
   requestOrThrow()
   new GraphQLCLient().request()
   new GraphQLCLient().requestOrThrow()
   new GraphQLCLient().rawRequest()
   new GraphQLCLient().rawRequestOrThrow()

• When throw is not used, make the return type of request be a union of HTTPError|TypedAggregateError<GraphQLError>|Data

• For rawRequest make it be GraphQLRequestResponse< HTTPError|TypedAggregateError<GraphQLError>|Data>

• Add an error mode configuration that allows GraphQL errors to coexist with data. request({ errorMode:'coexist'|'exclusive'}). When 'coexist' is used, then the return types change such that data and errors are together in an envelope: { data: Data, error: TypedAggregateError<GraphQLError> }.

• Note: TypedAggregateError is to handle multiple GraphQL errors in a type safe way. AggregateError has any[] for the errors field.

• make exclusive mode be default

In summary:

Exclusive Mode (default)

request()             | GraphQLRequestHTTPError
                      | TypedAggregateError<GraphQLError>
                      | Data


rawRequest()          GraphQLRequestResponse<
                       | GraphQLRequestHTTPError
                       | TypedAggregateError<GraphQLError>
                       | Data
                      >

requestOrThrow()      Data

rawRequestOrThrow()   GraphQLRequestResponse<Data>

Coexist Mode

request()           | GraphQLRequestHTTPError                                              
                    | {error: TypedAggregateError<GraphQLError>; data: Data }                                                                           

rawRequest()        GraphQLRequestResponse<
                      | GraphQLRequestHTTPError																						   
                      | {error: TypedAggregateError<GraphQLError>; data: Data }
                    >

requestOrThrow()    Data																																	

rawRequestOrThrow() GraphQLRequestResponse<Data> 																					

[AlexIII]

Is this subject to a vote? I am strongly in favor of rawRequest() always throwing, as it was before, and as it is now. Our code base deepens on it and a breaking change not only will force refactoring. I think throwing an exception should be default behavior, as it it not only a widely accepted error handling method, it is the one that is offered by the language itself, and the one that is used consistently in all relatively recent additions to the standard library.
I can see that in some cases, it's maybe more convenient to receive an error via return value, but it shouldn't be the default behavior.

[P4sca1]

Proposal for exclusive mode: https://github.com/supermacro/neverthrow
I think it is fine not to throw by default, because GraphQL is designed around the idea, that errors are possible and only partial data is returned. The orThrow naming is also easy to understand.

[jasonkuhrt]

@AlexIII IMO Throw is a language mistake. That TS doesn’t type it (understandably) exacerbates its faults. I’m aware it will impact existing users, but it’s strictly an improvement to not throw by default. And the refactor is straight forward, a few second find replace.

[jasonkuhrt]

@P4sca1 Cool looking library, but if we were to bless a library data structure returned by this library I would probably look to use the fp-ts ecosystem which now also include Effect.

[AlexIII]

@jasonkuhrt While I agree that generally, in programming, exceptions have proven to be inferior to the functional style, discussing shortcomings of the language is a bit out of the scope of one single library. No language is perfect, but the least we can do is to maintain consistency. I don't believe that refusing to use exception even in the most popular of libraries will ever EVER affect the language specification and the standard library (which everyone is forced to use weather one like it or not).

I do realize you coming that hard on the question, there's probably no way to persuade. Still, I find it necessary to voice this argument.

[jasonkuhrt]

Not throwing by default is an approach Prisma takes in some of its APIs, including an *orThrow suffix set of methods. Point being it's not a fringe approach and you con find it around the ecosystem, probably more and more rather than less and less.

I think making graphql-request output an fp-ts Either type would be a step too far in opinionation for example, but I don't think making throw be more explicit is.

I appreciate the thoughts and for-the-record capture of it here though thanks!

[beeman]

I'd love to see some improvement in the error handling!

Do you intend to address this quirk too? #201

[jasonkuhrt]

@beeman Generally yes. If there is a mistake this proposal repeats from there, or an aspect not addressed by this proposal, let me know!

[jasonkuhrt]

I think we can add a settings system that would allow the libraries default behaviour to be configured per what @AlexIII is asking for. There would still be a default-default but I'm totally open to being able to change the default globally.

[jasonkuhrt]

I am closing this as the WIP ts-client has a version of this going even further to account for schema errors. What people today know about graphql-request will become the "raw" client. The current API will not be changed but instead turned into a deprecated unchanging state starting in the next release with focus going thereafter toward the TS client.