Include original response in client errors#1476
Include original response in client errors#1476jasonkuhrt merged 5 commits intograffle-js:graphql-requestfrom
Conversation
By including the original fetch response when returning a ClientError call sites can use the full response to debug why the error happened. This opens up for e.g. utilizing api's that might at any time return a non-graphql response when an error occurs.
|
Thanks for the PR @adambrgmn! I've pushed a refactored approach that I think better addresses your use case. Changes MadeInstead of exposing the full interface GraphQLResponse<T = unknown> {
// ... existing fields
headers: Headers // ✅ Access response headers (e.g., Kill-Switch)
body: string // ✅ The response body text for non-GraphQL responses
}Why This Approach?
Test AddedI added a test case that validates the exact use case from your issue - accessing custom headers and parsing non-GraphQL response bodies. Does this approach work for your use case? If you need anything else from the Response object that I missed, let me know! |
Instead of exposing the full fetch Response (whose body stream is already consumed), expose the useful data directly on GraphQLResponse: - headers: Headers - for accessing response headers like Kill-Switch - rawBody: string - the raw response text for non-GraphQL responses This avoids the "consumed body stream" footgun while still enabling the use case from issue graffle-js#1473 (accessing non-GraphQL error responses).
9809932 to
6ead4b5
Compare
adambrgmn
left a comment
There was a problem hiding this comment.
That improvement is great. My solution was very basic. I did res.clone().json() in my approach since it was already read. But I'm sure this is more efficient and it will suit our use-case well 👍
Update tests to account for the new `body` property on GraphQLResponse: - Destructure `body` out of results in equality assertions - Update inline snapshots to include the body field
- Add body property to GraphQLClientResponse type (was only on GraphQLResponse) - Remove toBeInstanceOf(Headers) check that fails in jsdom environment
This PR adds the original fetch response to client errors to be able to act on non-graphql responses returned by the api.
The reason we need this is because we integrate with an api that might at any time return a non-graphql response in case of an error. And the body returned might hold reasons for the error which can't be parsed by the graphql parser.
We have tested this as a local patch for a while and think it is very useful, hence this PR.
This is connected to issue #1473.