rawRequest() return type wrong: can never include errors

[jamesharv]

The return type signature of rawRequest() includes errors?: GraphqlError[]

https://github.com/prisma-labs/graphql-request/blob/4b02516fc3707f00bd27ddd724a190bd78a32beb/src/index.ts#L16-L25

However, when processing the result of the operation the function explicitly will not return if the result contains any errors. Instead it throws a new ClientError:

https://github.com/prisma-labs/graphql-request/blob/4b02516fc3707f00bd27ddd724a190bd78a32beb/src/index.ts#L71-L76

So it seems to me that the return type is incorrect, and this has led to confusion in my team about how to correctly handle errors. The correct way is with try/catch, not by checking whether there is an errors property in the return value.

I think the solution is to just remove errors from the return type.

[IttisutBm]

I don't agree with your solution because many libs are not working in this way such as apollo.

The common way which is most libs do is:

if (response with status code 200 and have errors){
 return result // {data,errors}=result
}
if not (response with status code 200){
 throw exception
}

So in rawRequest()
It should be like this.

...
 if (response.ok && result.data) {
      const { headers, status } = response
      return { ...result, headers, status }
    } 
...

Additionally, This bug is also happens in request().
https://github.com/prisma-labs/graphql-request/blob/c75a29a9a2a177b0cddb41718b333fb14c9d9917/src/index.ts#L79
https://github.com/prisma-labs/graphql-request/blob/c75a29a9a2a177b0cddb41718b333fb14c9d9917/src/index.ts#L97-L102
So in request()
It should be like this.

...
 if (response.ok && result.data) { 
   return result
 } 
...

[jasonkuhrt]

I would like to get more feedback about if people prefer thrown error or error as data.

I am in favour of error as data.

Maybe there should be a config option which adjusts the API behaviour.

new GraphQLClient({ throwOnClientError: true })
// or
new GraphQLClient({ clientErrorsAsData: true })

Then use conditional types to maintain an accurately typed API.

[Evesy]

We are currently trying to debug some issues we have interacting with a 3rd party GraphQL API. I was hoping to use rawRequest as we need to extract a specific response header on requests that are failing (response with 200 status with errors).
This doesn't seem possible currently as the client instead throws an error and when catching this only the error data is accessible, not any of the raw request details. With this in mind, I'd be in favour of exposing client errors as data

[benwis]

I also vote for errors as data as implemented in #174

If it helps, I can try to do the config option mentioned earlier and submit another pull request.

[jasonkuhrt]

If it helps, I can try to do the config option mentioned earlier and submit another pull request.

That would be most welcome

[AlexIII]

I think the most important here is to fix the return type signature, as it is misleading. You can decide on how to handle errors later, but I would prefer there's no breaking changes, so error handling method should either be configurable or left as is now (i.e. throwing an exception).

[jasonkuhrt]

@AlexIII PR welcome

[AlexIII]

@jasonkuhrt ok, here it is. No breaking changes. Let me know if something's wrong.

[bobvanderlinden]

Same issue here. I presumed rawRequest would not throw exceptions upon receiving errors, as the type signature shows it returns errors in its return type. Good to see at least the type signature is being fixed.

[AlexIII]

Good news! We've got the return type fixed.

To those who want to see errors in the return type - please reconsider. The language already has a built-in method for handling errors, namely, exceptions. There's simply no good reason not to use them, as they lead to much cleaner code (common error interface, error propagation without boilerplate, handling errors in one place, etc.).

[implosivemosaic]

I just ran into this problem as well because it looks like graphql-codegen is still generating code that includes the GraphQLError type. It's great that this library has removed the ambiguity though, so at least now I can see pretty readily that we need to do this:

try {
  // some query...
catch (error) {
  if (error instanceof ClientError) {
    // handle specifics, for example:
    console.error(error.message)
  } else {
    // possibly rethrow or do other things here
  }
}

I think it would be worth reconsidering try/catch versus explicit return types. A lot of GraphQL APIs end up using the extensions object to surface errors, including those that are domain-specific (for instance a constraint violation when a user tries to create a record with a duplicate name). This means if I want to handle the error in my domain code I have to wrap the invocation as per the above before I can extract the relevant extension information from the errors object. My feeling is that this results in (a) annoying boilerplate, (b) less type safety (thrown errors have no type information) and (c) occasionally puzzling control flow issues (where did that error come from? why didn't this code run?).

I really like the simplicity of the library and so I'm providing this example as consideration for building on that simplicity.

[P4sca1]

The reteurn types of makeRequest and rawRequest include extensions?: any, but the only thing that is ever returned is data, status and headers or am I missing something?

[chrisdhanaraj]

Good news! We've got the return type fixed.

To those who want to see errors in the return type - please reconsider. The language already has a built-in method for handling errors, namely, exceptions. There's simply no good reason not to use them, as they lead to much cleaner code (common error interface, error propagation without boilerplate, handling errors in one place, etc.).

Quick comment - the reason why it's here is because it's pretty common in larger (federated) schemas to have partial results - this code block here

 if (response.ok && !result.errors && result.data) {
      const { headers, status } = response
      return { ...result, headers, status }
    } else {
      const errorResult = typeof result === 'string' ? { error: result } : result
      throw new ClientError(
        { ...errorResult, status: response.status, headers: response.headers },
        { query, variables }
      )
    }

Really should be returning the result no matter what if it's truly supposed to be the raw request, not a processed request.

[wesley079]

Hey there! Is this issue resolved by this PR? #352

[P4sca1]

Hey there! Is this issue resolved by this PR? #352

Yes, thank you for your PR!. There is still the issue that the errors property should be never depending on the configured error policy.

[jasonkuhrt]

I will work on better TS types soon.

[jasonkuhrt]

Actually I'm going to overhaul the error system: #509.