-
Notifications
You must be signed in to change notification settings - Fork 308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better error handling #509
Comments
Is this subject to a vote? I am strongly in favor of |
Proposal for exclusive mode: https://github.com/supermacro/neverthrow |
@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. |
@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. |
@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. |
Not throwing by default is an approach Prisma takes in some of its APIs, including an I think making I appreciate the thoughts and for-the-record capture of it here though thanks! |
I'd love to see some improvement in the error handling! Do you intend to address this quirk too? #201 |
@beeman Generally yes. If there is a mistake this proposal repeats from there, or an aspect not addressed by this proposal, let me know! |
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. |
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. |
Perceived Problem
ignore
which is confusingabc
as a url leads to a thrown fetch error ofTypeError: Only absolute URLs are supported
.Ideas / Proposed Solution(s)
Remove error policy configuration
Make
OrThrow
function variantsMake current functions default to not throwing (BREAKING CHANGE)
When throw is not used, make the return type of
request
be a union ofHTTPError|TypedAggregateError<GraphQLError>|Data
For
rawRequest
make it beGraphQLRequestResponse< 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 thatdata
anderrors
are together in an envelope:{ data: Data, error: TypedAggregateError<GraphQLError> }
.Note:
TypedAggregateError
is to handle multiple GraphQL errors in a type safe way.AggregateError
hasany[]
for theerrors
field.make
exclusive
mode be defaultIn summary:
Exclusive Mode (default)
Coexist Mode
The text was updated successfully, but these errors were encountered: