Skip to content
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

Remove didReceiveResponse hook #107

Merged
merged 5 commits into from
Dec 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .changeset/khaki-cars-remember.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
'@apollo/datasource-rest': major
---

Remove `didReceiveResponse` 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
https://github.com/apollographql/apollo-server/issues/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.
5 changes: 0 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -154,11 +154,6 @@ override cacheOptionsFor() {
}
```

##### `didReceiveResponse`
By default, this method checks if the response was returned successfully and parses the response into the result object. If the response had an error, it detects which type of HTTP error and throws the error result.

If you override this behavior, be sure to implement the proper error handling.

##### `didEncounterError`
By default, this method just throws the `error` it was given. If you override this method, you can choose to either perform some additional logic and still throw, or to swallow the error by not throwing the error result.

Expand Down
19 changes: 7 additions & 12 deletions src/RESTDataSource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,17 +176,6 @@ export abstract class RESTDataSource {
request: FetcherRequestInit,
): CacheOptions | undefined;

protected async didReceiveResponse<TResult = any>(
response: FetcherResponse,
_request: RequestOptions,
): Promise<TResult> {
if (response.ok) {
return this.parseBody(response) as any as Promise<TResult>;
} else {
throw await this.errorFromResponse(response);
}
}

protected didEncounterError(error: Error, _request: RequestOptions) {
throw error;
}
Expand Down Expand Up @@ -353,9 +342,15 @@ export abstract class RESTDataSource {
cacheKey,
cacheOptions,
});
return await this.didReceiveResponse(response, outgoingRequest);

if (response.ok) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

return (await this.parseBody(response)) as TResult;
} else {
throw await this.errorFromResponse(response);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}
} catch (error) {
this.didEncounterError(error as Error, outgoingRequest);
throw error;
}
});
};
Expand Down