Remove didReceiveResponse hook

.changeset/khaki-cars-remember.md

@@ -0,0 +1,20 @@
+---
+'@apollo/datasource-rest': major
+---
+
+Reduce responsibility of `didReceiveResponse` hook
+
+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
+https://github.com/apollographql/apollo-server/issues/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.

src/RESTDataSource.ts

@@ -176,16 +176,10 @@ export abstract class RESTDataSource {
  request: FetcherRequestInit,
  ): CacheOptions | undefined;
 
- protected async didReceiveResponse<TResult = any>(
+ protected async didReceiveResponse?(
  response: FetcherResponse,
- _request: RequestOptions,
- ): Promise<TResult> {
- if (response.ok) {
- return this.parseBody(response) as any as Promise<TResult>;
- } else {
- throw await this.errorFromResponse(response);
- }
- }
+ request: RequestOptions,
+ ): Promise<void>;
 
  protected didEncounterError(error: Error, _request: RequestOptions) {
  throw error;
@@ -353,9 +347,19 @@ export abstract class RESTDataSource {
  cacheKey,
  cacheOptions,
  });
- return await this.didReceiveResponse(response, outgoingRequest);
+
+ if (this.didReceiveResponse) {
+ await this.didReceiveResponse(response.clone(), outgoingRequest);
+ }
+
+ if (response.ok) {
+ return (await this.parseBody(response)) as TResult;
+ } else {
+ throw await this.errorFromResponse(response);
+ }
  } catch (error) {
  this.didEncounterError(error as Error, outgoingRequest);
+ throw error;
  }
  });
  };

src/__tests__/RESTDataSource.test.ts

@@ -13,6 +13,7 @@ import FormData from 'form-data';
 import { GraphQLError } from 'graphql';
 import nock from 'nock';
 import { nockAfterEach, nockBeforeEach } from './nockAssertions';
+import type { FetcherResponse } from '@apollo/utils.fetcher';
 
 const apiUrl = 'https://api.example.com';
 
@@ -1145,6 +1146,47 @@ describe('RESTDataSource', () => {
  expect(calls).toBe(1);
  });
  });
+
+ describe('didReceiveResponse', () => {
+ it('is called if implemented', async () => {
+ const didReceiveResponseMock = jest.fn(async () => {});
+ const dataSource = new (class extends RESTDataSource {
+ override baseURL = apiUrl;
+
+ getFoo(id: number) {
+ return this.get(`foo/${id}`);
+ }
+
+ override didReceiveResponse = didReceiveResponseMock;
+ })();
+
+ nock(apiUrl).get('/foo/1').reply(200, { name: 'bar' });
+ await dataSource.getFoo(1);
+ expect(didReceiveResponseMock).toHaveBeenCalledTimes(1);
+ });
+
+ it('is ok to consume the body on the response', async () => {
+ let observedBody;
+ const dataSource = new (class extends RESTDataSource {
+ override baseURL = apiUrl;
+
+ getFoo(id: number) {
+ return this.get(`foo/${id}`);
+ }
+
+ override async didReceiveResponse(
+ response: FetcherResponse,
+ _outgoingRequest: RequestOptions,
+ ) {
+ observedBody = await response.json();
+ }
+ })();
+
+ nock(apiUrl).get('/foo/1').reply(200, { name: 'bar' });
+ await dataSource.getFoo(1);
+ expect(observedBody).toEqual({ name: 'bar' });
+ });
+ });
  });
  });
 });