didReceiveResponse hook no longer available, blocking Apollo v4 upgrade

[JudgeGreg]

We are upgrading an Apollo server to v4 and noticed that in datasource-rest v5 didReceiveResponse hook was removed https://github.com/apollographql/datasource-rest/releases/tag/v5.0.0 .

With our current codebase we use that hook for adding some headers which would be fine with moving to the parseBody hook however, we also do some logging there that relies on both the request and response objects that were available in didReceiveResponse previously and are not in parseBody.

4b2a6f9 This commit message implies that this hook could be reinstated and we found a valid use case for it too. At the moment this is a blocker for us - could you consider adding this hook back and if yes, approximately when can we expect it?

Thank you

[dycor]

It's a blocker for me too

[trevor-scheer]

Hey @grego-e and @dycor. Have you taken a look at #109? I do think overriding fetch in the way I've suggested is sufficient for your use case (headers and logging). I won't claim it's the ideal ergonomic but hopefully unblocking for now at least.

It seems like there's enough interest here that I'm tempted to reinstate the hook, but I'm a bit hesitant only because people upgrading need to be aware of the subtle-ish change in semantics. I guess that's why we read changelogs, especially through major version upgrades.

I'd be open to a PR which adds some form of the hook back. Maybe willSendResponsewillReturnResponse is really what people want? I'm imagining a hook into the final result before it's returned with the parsedBody and response made available to the hook.

cc @glasser

[glasser]

I do feel like if the use case is "do something different right at the end of fetch" then overriding fetch in your subclass seems reasonable. The main downside here is if your overrided method would need access to anything other than the return value from super.fetch or the initial arguments to fetch (ie, something computed internally). Is that the case here? We could return more things from fetch if that helps.

[dycor]

I need to access to the headers response , and with didReceiveResponse I can do this

[trevor-scheer]

@dycor did you read #109?

In my suggestion you can access the response headers like so:

class MyDataSource extends RESTDataSource {
  override async fetch<TResult>(
    path: string,
    incomingRequest: DataSourceRequest = {},
  ) {
    return super.fetch<TResult>(path, incomingRequest).then((result) => {
      const response = result.response;
      const headers = response.headers;

      if (!response.ok) {
        // mutate response object accordingly
      }
      return result;
    });
  }
}

[JudgeGreg]

Thanks for the fetch suggestion @trevor-scheer, we ended up overriding it.