Remove didReceiveResponse hook
#107
Conversation
The naming of this hook is deceiving; if this hook is overridden it becomes responsible for returning the parsed body. It was originally introduced in apollographql/apollo-server#1324, where the author claims they implemented it due to lack of access to the complete response (headers) in the fetch methods (get, post, ...). This approach isn't a type safe way to acoomplish this. This hook is now just an observability hook which receives a clone of the response and the request that was sent. A change following this will introduce the ability to fetch a complete response (headers included) aside from the provided fetch methods which only return a body, which will reinstate the functionality that the author of this hook had originally intended.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
src/RESTDataSource.ts
Outdated
| return await this.didReceiveResponse(response, outgoingRequest); | ||
|
|
||
| if (this.didReceiveResponse) { | ||
| await this.didReceiveResponse(response.clone(), outgoingRequest); |
There was a problem hiding this comment.
Thought 1: why a clone? Would it be valuable to be able to use this hook to mutate the response if you want? (And it does affect performance negatively to do so.)
src/RESTDataSource.ts
Outdated
| return await this.didReceiveResponse(response, outgoingRequest); | ||
|
|
||
| if (this.didReceiveResponse) { | ||
| await this.didReceiveResponse(response.clone(), outgoingRequest); |
There was a problem hiding this comment.
Thought 2: Seems like another case where we need to include url alongside outgoingRequest (not new in this PR, just while we're here)
| await this.didReceiveResponse(response.clone(), outgoingRequest); | ||
| } | ||
|
|
||
| if (response.ok) { |
There was a problem hiding this comment.
While I do think that didReceiveResponse was the wrong place for this logic, it does kinda feel like it would be helpful for users to be able to differentiate between the error and not cases (eg, maybe you have an API where you want to do non-error for some specific 4xx response, idk). Not sure if this means adding yet another hook, or leaving didReceiveResponse as kinda similar to current semantics but with a better name, or what. (Or just waiting until a user says they can't upgrade without this feature.)
| if (response.ok) { | ||
| return (await this.parseBody(response)) as TResult; | ||
| } else { | ||
| throw await this.errorFromResponse(response); |
There was a problem hiding this comment.
completely unrelated: the body of errorFromResponse could be rewritten to just pass the response extension directly to the error constructor instead of being written how it is now (and maybe drop the subclasses and put the error extensions directly into the GraphQLError call, idk)
trevor-scheer
changed the title
didReceiveResponse hookdidReceiveResponse hook
The naming of this hook is deceiving; if this hook is overridden it becomes
responsible for returning the parsed body and handling errors if they occur. It
was originally introduced in
apollographql/apollo-server#1324, where the author
implemented it due to lack of access to the complete response (headers) in the
fetch methods (get, post, ...). This approach isn't a type safe way to
accomplish this and places the burden of body parsing and error handling on the
user.
Removing this hook is a prerequisite to a subsequent change that will introduce
the ability to fetch a complete response (headers included) aside from the
provided fetch methods which only return a body. This change will reinstate the
functionality that the author of this hook had originally intended in a more
direct manner.
We can consider reinstating this hook or something similar in the future with well-documented and use-case-driven semantics. Without knowing other reasons for why this hook should exist (aside from mentioned issue, which will be addressed with an alternative approach), trying to guess what this hook should do or mean is a bit pointless.