Errors stop getting sent to client if the override of fetch includes a catch callback

[AlphaSteam]

I was overriding the fetch method to replicate the functionality I had with didReceiveResponse , which is logging each request. For that I need to be able to differentiate and log both successful and unsuccessful requests.

Using the fix in #109 let's me log the successful requests.

The problem arises when trying to log the unsuccessful ones. I tried adding a catch callback as follows:

return super.fetch<TResult>(path, incomingRequest).then(onFulfilled).catch(onRejected);

The function onRejected is:

const onRejected = (reason: any) => {
      if (reason?.extensions?.response) {
        const { url, status, statusText } = reason?.extensions?.response;
        const { method } = incomingRequest;

        const parsedURL = new URL(url);

        logger.error("
method: ${method}
status: ${status}
statusText: ${statusText}
service: ${parsedURL.hostname}
path: ${parsedURL.pathname}");
      }
      return reason;
    }; 

incomingRequest and url come from fetch.
This logs the unsuccessful requests, but makes it so the client stops receiving error information. Without the catch, apollo server returns this response:

{
 "data": {
   "login": null
 },
 "errors": [
   {
     "status": 401,
     "statusText": "Unauthorized",
     "message": "Error logging in. Code has expired"
   }
 ]
}

With it, it responds with:

{
  "data": {
   "login": null
  }
}

[AlphaSteam]

Fixed it by removing the catch callback and overriding throwIfResponseIsError like so:

protected override async throwIfResponseIsError(
    options: {
      url: URL;
      request: RequestOptions;
      response: FetcherResponse;
      parsedBody: unknown;
    }): 
    Promise<void> {
    const {
      request: { method },
      response: { status, statusText },
      url
    } = options;

        if (!options.response.ok) {
      logger.error(logMessage({ method, status, statusText, url }));
    }

    return super.throwIfResponseIsError(options);
  }

I think it should be added to the docs in #145. As also logging unsuccessful requests is a possible common use case if one wants to log the successful ones.

At the same time I'm not entirely sure why catching fetch breaks the response to the client. I guess it happens because catching fetch directly bypasses throwIfResponseIsError?

[trevor-scheer]

@AlphaSteam I think it's a good suggestion to add the error handling case to my PR as well.

The reason that breaks is because you're suppressing the error by catching it. If you'd like to handle the error in a catch you need to rethrow it at the end of the catch block.

If you just want to observe the error / do some logging, you can use the didEncounterError hook. You can also see how the catch/rethrow is being done for you in the code.

datasource-rest/src/RESTDataSource.ts

Lines 533 to 536 in c9ffa7f

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

I think what you're doing is fine with throwIfResponseIsError though its intended use is to override default behavior (when to throw errors), not just observe the error that occurred. As a result, the ergonomic is not as good for your use case (you have to return super.throwIfResponseError(...)). There are some more details in the PR that introduced it:
#115