Refactor errorFromResponse

[lotmek]

Description

• Turn url and request into optional parameters
• Refactor the way to add code in the extensions object

Fixes #247

[codesandbox-ci]

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.

[trevor-scheer]

I think this is a good change, especially since the concrete method here doesn't make use of those params. Thinking out loud because at first glance, this seems like a breaking change. Explaining why it isn't.

The signature of the method does not impose any requirements on the signature in concrete class definitions that extend RESTDataSource and override this method. That means an overridden errorFromResponse method can have any signature, meaning users who currently extend errorFromResponse will not be affected by this change. This isn't the first time I've "discovered" this, but usually surprises me.

Additionally, users currently calling this method are unaffected - they won't see any change for already including parameters which become optional (though the other direction optional -> required would be problematic).

Related thought worth capturing that doesn't impact this PR:
throwIfResponseIsError and errorFromResponse are "linked" by their types. If you override errorFromResponse and provide a different signature (specifically, an extended one), you're going to be surprised when throwIfResponseIsError calls your method. I'm actually not sure how to fix this.

class MyDataSource extends RESTDataSource {
  override async errorFromResponse({
    response,
    parsedBody,
  }: {
    response: FetcherResponse;
    parsedBody: string;
    extra: string; // not a TS error, but this won't exist if I don't override `throwIfResponseIsError`
  }): Promise<GraphQLError> {
    return new GraphQLError('...');
  }
}

[trevor-scheer]

Please add a patch changeset describing your changes. You can run the changeset tool via npx changeset and follow the prompts.

[trevor-scheer]

This change looks correct to me but without a test validating your change I'd rather just leave it as is. I don't think the ternaries here are so complex that it necessitates cleanup.

My preference is to revert this, but if you insist then I'd require a test.

[lotmek]

Hi @trevor-scheer the codes are already tested in those two cases:

• throws a FORBIDDEN error when the response status is 403
• throws an UNAUTHENTICATED error when the response status is 401

I am just adding an extra assertion in this third test case (throws an ApolloError when the response status is 500), just to make sure the property is not being added:

datasource-rest/src/__tests__/RESTDataSource.test.ts

Line 1641 in 410213b

await expect(result).rejects.not.toHaveProperty('extensions.code');