Allow overriding URL/method in RemoteGraphQLDataSource

[Cretezy]

I'm currently attempting to override the fetched URL/method in RemoteGraphQLDataSource. Although it's possible with a custom fetcher, it really should be easier.

I'm currently attempting to do something like:

return new RemoteGraphQLDataSource<SharedContext>({
  url,
  willSendRequest({ request, context }) {
    if(context.urlOverride && request.http) {
      request.http.url = context.urlOverride;
    }
  },
});

However, due to the readonly modifier on Request.url, this is impossible, even though it would work as per the code.

method should also work the same.

From what I can see by a brief look, this would not affect any other parts of the application (plus, it is a type-only change).

[apollo-cla]

@Cretezy: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[glasser]

This seems reasonable, but even better would be eliminating our own hand-coded copies of the node-fetch types altogether, as I'm considering over in #5515 (comment) ...

[Cretezy]

@glasser I'd be fine with that too. I can help with reviews/testing if needed.

Just want to get this fixed so I can avoid // @ts-ignore

[glasser]

@abernix thoughts on this (moved over from #5515)? It does look like these fields are readonly in lib.dom.d.ts, so I'm not sure if this change makes sense — maybe there's another way to implement this?

[glasser]

@abernix It seems like the options are:

• Focus in on doing what node-fetch supports, update the types as given here, understand that this will be less compatible with other Fetch implementations
• Add another context-sensitive hook to RemoteGraphQLDataSource that lets you replace the request or perhaps just the URL

[abernix]

Focus in on doing what node-fetch supports, update the types as given here, understand that this will be less compatible with other Fetch implementations

This seems like the opposite direction than what we've been trying to move in since node-fetch is very restrictive and limited in what it offers.

maybe there's another way to implement this?

Seems somewhat reasonable for the properties to not be readonly on the http object. Maybe we just -[readonly] them?

Or http itself isn't readonly – could just assign a new Request to it?

[glasser]

Or http itself isn't readonly – could just assign a new Request to it?

Hmm, @abernix, that seems attractive but it looks like the Request API annoyingly doesn't have an API for "copy this Request but with a different url"? clone takes no arguments, and you can provide a Request to the Request constructor but only in the same spot as the url!

We could just add some sort of getUrl({ request, context }) to RemoteGraphQLDataSource...

[glasser]

I think that moving our TS types farther away from the actual Fetch API isn't a good idea. I'm going to transfer this issue to the federation repo and suggest we think of it as a suggestion for providing an overridable hook to provide the URL/request per-request.

eg, we could extract out

    const headers = (request.http && request.http.headers) || new Headers();
    headers.set('Content-Type', 'application/json');

    request.http = {
      method: 'POST',
      url: this.url,
      headers,
    };

into something like

  request.http = this.makeRequest({ request, context });

which you can override as you'd like.

(Note that #870 adds an option to process and we should probably just pass the full argument to process to this new hook.)

[glasser]

Or well, this is a PR, so I'm going to close this and open an issue there.

[glasser]

Filed apollographql/federation#901