Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix fetch customization, upgrade make-fetch-happen, more concurrency #1805

Merged
merged 1 commit into from
Apr 29, 2022

Conversation

glasser
Copy link
Member

@glasser glasser commented Apr 29, 2022

UplinkFetcher and RemoteGraphQLDataSource both use make-fetch-happen
to execute HTTP requests and allow you to replace that implementation
with any compatible implementation of the web Fetch API.

But prior to this PR, this customization often did not work. For
example, prior to this PR, the version of make-fetch-happen used by
default was the outdated v8. If you wanted to use the current v10, you'd
think you could just install it in your project and pass its fetch
function to the appropriate fetcher parameter.

But surprisingly, this would not work for RemoteGraphQLDataSource! The
Fetch API is flexible and allows you to specify your request either as
plain JSON-style objects or as instances of the Request and Headers
classes defined in the Fetch API spec. RemoteGraphQLDataSource used a
Headers object in its requests. Specifically, it used a Headers
object imported at runtime from apollo-server-env, a package we wrote
that combines custom TypeScript types with node-fetch for runtime
behavior.

As it turns out, there's a behavior change in make-fetch-happen v9 as
to how it tells the difference between using its API with JSON-style
arguments or the Requests/Headers class. In the newer version,
make-fetch-happen uses an instanceof check to see if the provided
object is its Headers class. So when RemoteGraphQLDataSource
passes in the node-fetch Headers class, make-fetch-happens v9 gets
confused (and treats it as an empty JSON-style object instead).

The solution we've settled on is to avoid passing non-plain objects to
any fetch call that we intend to be customizable. We've published a
new package, @apollo/utils.fetcher, which provides TypeScript
definitions for a subset of the Fetch API that does not include the
dangerous feature of "passing in Request or Headers objects". This
PR changes both uses of fetcher in Gateway to use this new TypeScript
type and to invoke it in a more conservative way. (We're making a
similar change in the upcoming Apollo Server 4.)

This does affect the types passed to some RemoteGraphQLDataSource
methods, which are designed to be overridden. We feel reasonably sure
that this won't have any noticeable impact on subclasses, as the changed
types are intended to be very similar to the types they are replacing.

Semi-relatedly, we've changed the default fetcher used by
RemoteGraphQLDataSource to allow arbitrarily many parallel connections
to each subgraph rather than only 15. maxSockets: 15 is the default
for make-fetch-happen (though not for the underlying agentkeepalive
or http.Agent libraries) and so this limit was unintentionally added
to Gateway when we switched from node-fetch to make-fetch-happen.
While this change is a few years old now, it was unintentional and we
doubt anyone is relying on this; on the contrary, it is causing real
performance problems for production users (#1647). So we are changing
the default to unlimited. If for some reason it is important to you that
your Gateway only be able to process 15 requests at a time, you can
restore the previous behavior:

import fetcher from 'make-fetch-happen';
const lowConcurrencyFetcher = fetcher.defaults({ maxSockets: 15 });
const gateway = new ApolloGateway({
  buildService({ url }) {
    return new RemoteGraphQLDataSource({
      url,
      fetcher: lowConcurrencyFetcher,
    });
  },
});

Fixes #1647.
Fixes #1287.

In more detail, this change consists of:

  • Switch the TypeScript typings fetcher option to new ApolloGateway
    and new RemoteGraphQLDataSource from a function type defined in
    apollo-server-env to a new one defined in @apollo/utils.fetcher.
    This function type's return type is compatible with the return type of
    the old fetcher, but its argument types are restricted to a subset of
    the Fetch API that is actually used by these classes (and classes in
    Apollo Server 4). Specifically, they do not allow you to pass
    Request or Headers objects. Adjust all calls to pass only
    plain JSON objects rather than Request/Headers objects.

  • In RemoteGraphQLDataSource, change the type of the "fetch request"
    argument to parseBody and didEncounterError (which are ignored by
    that class's implementations of the methods but could be examined by a
    subclass's implementations) from apollo-server-env's Request type
    to node-fetch's. This Request object is actually created at
    runtime by RemoteGraphQLDataSource and is always actually a
    node-fetch object, so reflecting that in the API seems to be
    reasonable. Note that this object is not actually sent to the
    Fetcher; it is only sent to these subclass-observable methods. This
    change makes the TypeScript type used for these parameters match the
    actual runtime type precisely so it should be a backwards-compatible
    change.

  • In RemoteGraphQLDataSource, change the type of the "fetch response"
    argument to parseBody, errorFromResponse, and didEncounterError
    from apollo-server-env's Response type to
    @apollo/utils.fetcher's FetcherResponse type. This interface type
    was designed to contain all the fields of the apollo-server-env type
    so this should be a backwards-compatible change.

  • Upgrade make-fetch-happen (the default fetcher used to talk to
    Uplink and to subgraphs) from v8 to v10. Stop preventing Renovate from
    upgrading it.

  • Changing the make-fetch-happen fetcher used in
    RemoteGraphQLDataSource to not limit the number of concurrent active
    sockets. (The fetcher used by UplinkFetcher keeps its defaults, as
    that fetcher does not need to make concurrent fetches.)

  • Remove the exported function getDefaultFetcher, as described in the
    CHANGELOG. None of the customizations we were making were actually
    still relevant for its use in UplinkFetcher so it seemed simpler to
    just remove the function rather than keep it around purely to allow
    people to simulate the way that an old version of UplinkFetcher
    fetched things by default. (We can add it back if there's a lot of
    protest, I suppose, although we'd be unlikely to make it continue to
    export make-fetch-happen v8!)

  • Replace tests that use Jest mocking of apollo-server-env and
    make-fetch-happen with nock or explicitly passed-in fetchers.
    This simplifies the tests and makes them less tied to the particular
    fetch implementations. (These tests were written before we knew about
    nock.) Also remove some code in IntrospectAndCompose.test.ts that
    disabled this mocking so that nock would work.

  • Remove the direct dependency on apollo-server-env. (Note that some
    types defined in apollo-server-env are still used indirectly in
    Gateway, because we still use apollo-server-types and types such as
    GraphQLRequest contain types from apollo-server-env.)

Paired with @trevor-scheer.

@netlify
Copy link

netlify bot commented Apr 29, 2022

Deploy Preview for apollo-federation-docs ready!

Name Link
🔨 Latest commit 55517da
🔍 Latest deploy log https://app.netlify.com/sites/apollo-federation-docs/deploys/626c7116367726000824a810
😎 Deploy Preview https://deploy-preview-1805--apollo-federation-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 29, 2022

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.

@glasser glasser changed the title checkpoint for Fetcher Improve fetcher customization and upgrade make-fetch-happen Apr 29, 2022
@glasser glasser changed the title Improve fetcher customization and upgrade make-fetch-happen Fix fetch customization, upgrade make-fetch-happen, more concurrency Apr 29, 2022
Copy link
Member

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick skim LGTM (though I helped author this code, so other eyes might be good here)

@@ -1,56 +0,0 @@
import {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

@glasser
Copy link
Member Author

glasser commented Apr 29, 2022

The main runtime change here is similar to #188. That PR had us stop passing Request objects to the fetcher but still included Headers objects. Now we will avoid doing both. (Turns out that avoiding foreign Request objects was necessary for make-fetch-happen v8 but foreign Headers objects happened to work back then and don't for newer versions.)

@glasser glasser force-pushed the glasser/fetcher-type branch 2 times, most recently from 40bbbde to b25c12a Compare April 29, 2022 23:10
@glasser glasser enabled auto-merge (squash) April 29, 2022 23:11
@glasser glasser disabled auto-merge April 29, 2022 23:11
UplinkFetcher and RemoteGraphQLDataSource both use `make-fetch-happen`
to execute HTTP requests and allow you to replace that implementation
with any compatible implementation of the web Fetch API.

But prior to this PR, this customization often did not work. For
example, prior to this PR, the version of `make-fetch-happen` used by
default was the outdated v8. If you wanted to use the current v10, you'd
think you could just install it in your project and pass its `fetch`
function to the appropriate `fetcher` parameter.

But surprisingly, this would not work for `RemoteGraphQLDataSource`! The
Fetch API is flexible and allows you to specify your request either as
plain JSON-style objects or as instances of the `Request` and `Headers`
classes defined in the Fetch API spec. `RemoteGraphQLDataSource` used a
`Headers` object in its requests. Specifically, it used a `Headers`
object imported at runtime from `apollo-server-env`, a package we wrote
that combines custom TypeScript types with `node-fetch` for runtime
behavior.

As it turns out, there's a behavior change in `make-fetch-happen` v9 as
to how it tells the difference between using its API with JSON-style
arguments or the `Requests`/`Headers` class. In the newer version,
`make-fetch-happen` uses an `instanceof` check to see if the provided
object is *its* `Headers` class. So when `RemoteGraphQLDataSource`
passes in the `node-fetch` `Headers` class, `make-fetch-happens` v9 gets
confused (and treats it as an empty JSON-style object instead).

The solution we've settled on is to avoid passing non-plain objects to
any `fetch` call that we intend to be customizable. We've published a
new package, `@apollo/utils.fetcher`, which provides TypeScript
definitions for a subset of the Fetch API that does not include the
dangerous feature of "passing in `Request` or `Headers` objects". This
PR changes both uses of `fetcher` in Gateway to use this new TypeScript
type and to invoke it in a more conservative way. (We're making a
similar change in the upcoming Apollo Server 4.)

This does affect the types passed to some `RemoteGraphQLDataSource`
methods, which are designed to be overridden. We feel reasonably sure
that this won't have any noticeable impact on subclasses, as the changed
types are intended to be very similar to the types they are replacing.

Semi-relatedly, we've changed the default fetcher used by
`RemoteGraphQLDataSource` to allow arbitrarily many parallel connections
to each subgraph rather than only 15. `maxSockets: 15` is the default
for `make-fetch-happen` (though not for the underlying `agentkeepalive`
or `http.Agent` libraries) and so this limit was unintentionally added
to Gateway when we switched from `node-fetch` to `make-fetch-happen`.
While this change is a few years old now, it was unintentional and we
doubt anyone is relying on this; on the contrary, it is causing real
performance problems for production users (#1647). So we are changing
the default to unlimited. If for some reason it is important to you that
your Gateway only be able to process 15 requests at a time, you can
restore the previous behavior:

    import fetcher from 'make-fetch-happen';
    const lowConcurrencyFetcher = fetcher.defaults({ maxSockets: 15 });
    const gateway = new ApolloGateway({
      buildService({ url }) {
        return new RemoteGraphQLDataSource({
          url,
          fetcher: lowConcurrencyFetcher,
        });
      },
    });

Fixes #1647.
Fixes #1287.

In more detail, this change consists of:

- Switch the TypeScript typings `fetcher` option to `new ApolloGateway`
  and `new RemoteGraphQLDataSource` from a function type defined in
  `apollo-server-env` to a new one defined in `@apollo/utils.fetcher`.
  This function type's return type is compatible with the return type of
  the old fetcher, but its argument types are restricted to a subset of
  the Fetch API that is actually used by these classes (and classes in
  Apollo Server 4). Specifically, they do not allow you to pass
  `Request` or `Headers` objects. Adjust all calls to pass only
  plain JSON objects rather than `Request`/`Headers` objects.

- In `RemoteGraphQLDataSource`, change the type of the "fetch request"
  argument to `parseBody` and `didEncounterError` (which are ignored by
  that class's implementations of the methods but could be examined by a
  subclass's implementations) from `apollo-server-env`'s `Request` type
  to `node-fetch`'s. This `Request` object is actually created at
  runtime by `RemoteGraphQLDataSource` and is always actually a
  `node-fetch` object, so reflecting that in the API seems to be
  reasonable. Note that this object is *not* actually sent to the
  `Fetcher`; it is only sent to these subclass-observable methods. This
  change makes the TypeScript type used for these parameters match the
  actual runtime type precisely so it should be a backwards-compatible
  change.

- In `RemoteGraphQLDataSource`, change the type of the "fetch response"
  argument to `parseBody`, `errorFromResponse`, and `didEncounterError`
  from `apollo-server-env`'s `Response` type to
  `@apollo/utils.fetcher`'s `FetcherResponse` type. This interface type
  was designed to contain all the fields of the `apollo-server-env` type
  so this should be a backwards-compatible change.

- Upgrade `make-fetch-happen` (the default fetcher used to talk to
  Uplink and to subgraphs) from v8 to v10. Stop preventing Renovate from
  upgrading it.

- Changing the `make-fetch-happen` fetcher used in
  `RemoteGraphQLDataSource` to not limit the number of concurrent active
  sockets. (The fetcher used by `UplinkFetcher` keeps its defaults, as
  that fetcher does not need to make concurrent fetches.)

- Remove the exported function `getDefaultFetcher`, as described in the
  CHANGELOG. None of the customizations we were making were actually
  still relevant for its use in `UplinkFetcher` so it seemed simpler to
  just remove the function rather than keep it around purely to allow
  people to simulate the way that an old version of `UplinkFetcher`
  fetched things by default. (We can add it back if there's a lot of
  protest, I suppose, although we'd be unlikely to make it continue to
  export `make-fetch-happen` v8!)

- Replace tests that use Jest mocking of `apollo-server-env` and
  `make-fetch-happen` with `nock` or explicitly passed-in fetchers.
  This simplifies the tests and makes them less tied to the particular
  fetch implementations. (These tests were written before we knew about
  nock.) Also remove some code in IntrospectAndCompose.test.ts that
  *disabled* this mocking so that nock would work.

- Remove the direct dependency on `apollo-server-env`. (Note that some
  types defined in `apollo-server-env` are still used indirectly in
  Gateway, because we still use `apollo-server-types` and types such as
  `GraphQLRequest` contain types from `apollo-server-env`.)

Paired with @trevor-scheer.
@glasser glasser merged commit d900859 into main Apr 29, 2022
@glasser glasser deleted the glasser/fetcher-type branch April 29, 2022 23:17
@@ -4,7 +4,9 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The

## vNEXT

- The `fetch` implementation returned by `getDefaultFetcher` no longer performs in-memory caching. This fetcher is currently only used by `@apollo/gateway` to make uncacheable `POST` requests to Uplink, so this is a no-op for Gateway's own behavior, but if you used the function returned `getDefaultFetcher` in your own code to perform `GET` requests against servers that return cache-related response headers, it will no longer cache results. You can use the underlying [`make-fetch-happen`](https://www.npmjs.com/package/make-fetch-happen) package directly to use its cache capabilities instead of using the function returned by `getDefaultFetcher`. [PR #1792](https://github.com/apollographql/federation/pull/1792)
- The `fetch` implementation used by default by `UplinkFetcher` and `RemoteGraphQLDataSource` is now imported from `make-fetch-happen` v10 instead of v8. The fetcher used by `RemoteGraphQLDataSource` no longer limits the number of simultaneous requests per subgraph to 15 by default; instead, there is no limit. (If you want to restore the previous behavior, install `make-fetch-happen`, import `fetcher` from it, and pass `new RemoteGraphQLDataSource({ fetcher: fetcher.defaults(maxSockets: 15)}))` in your `buildService` option.) [PR #1806](https://github.com/apollographql/federation/pull/1806)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops these PR #s are wrong (should be 1805) @glasser

trevor-scheer pushed a commit that referenced this pull request Apr 29, 2022
…1805)

UplinkFetcher and RemoteGraphQLDataSource both use `make-fetch-happen`
to execute HTTP requests and allow you to replace that implementation
with any compatible implementation of the web Fetch API.

But prior to this PR, this customization often did not work. For
example, prior to this PR, the version of `make-fetch-happen` used by
default was the outdated v8. If you wanted to use the current v10, you'd
think you could just install it in your project and pass its `fetch`
function to the appropriate `fetcher` parameter.

But surprisingly, this would not work for `RemoteGraphQLDataSource`! The
Fetch API is flexible and allows you to specify your request either as
plain JSON-style objects or as instances of the `Request` and `Headers`
classes defined in the Fetch API spec. `RemoteGraphQLDataSource` used a
`Headers` object in its requests. Specifically, it used a `Headers`
object imported at runtime from `apollo-server-env`, a package we wrote
that combines custom TypeScript types with `node-fetch` for runtime
behavior.

As it turns out, there's a behavior change in `make-fetch-happen` v9 as
to how it tells the difference between using its API with JSON-style
arguments or the `Requests`/`Headers` class. In the newer version,
`make-fetch-happen` uses an `instanceof` check to see if the provided
object is *its* `Headers` class. So when `RemoteGraphQLDataSource`
passes in the `node-fetch` `Headers` class, `make-fetch-happens` v9 gets
confused (and treats it as an empty JSON-style object instead).

The solution we've settled on is to avoid passing non-plain objects to
any `fetch` call that we intend to be customizable. We've published a
new package, `@apollo/utils.fetcher`, which provides TypeScript
definitions for a subset of the Fetch API that does not include the
dangerous feature of "passing in `Request` or `Headers` objects". This
PR changes both uses of `fetcher` in Gateway to use this new TypeScript
type and to invoke it in a more conservative way. (We're making a
similar change in the upcoming Apollo Server 4.)

This does affect the types passed to some `RemoteGraphQLDataSource`
methods, which are designed to be overridden. We feel reasonably sure
that this won't have any noticeable impact on subclasses, as the changed
types are intended to be very similar to the types they are replacing.

Semi-relatedly, we've changed the default fetcher used by
`RemoteGraphQLDataSource` to allow arbitrarily many parallel connections
to each subgraph rather than only 15. `maxSockets: 15` is the default
for `make-fetch-happen` (though not for the underlying `agentkeepalive`
or `http.Agent` libraries) and so this limit was unintentionally added
to Gateway when we switched from `node-fetch` to `make-fetch-happen`.
While this change is a few years old now, it was unintentional and we
doubt anyone is relying on this; on the contrary, it is causing real
performance problems for production users (#1647). So we are changing
the default to unlimited. If for some reason it is important to you that
your Gateway only be able to process 15 requests at a time, you can
restore the previous behavior:

    import fetcher from 'make-fetch-happen';
    const lowConcurrencyFetcher = fetcher.defaults({ maxSockets: 15 });
    const gateway = new ApolloGateway({
      buildService({ url }) {
        return new RemoteGraphQLDataSource({
          url,
          fetcher: lowConcurrencyFetcher,
        });
      },
    });

Fixes #1647.
Fixes #1287.

In more detail, this change consists of:

- Switch the TypeScript typings `fetcher` option to `new ApolloGateway`
  and `new RemoteGraphQLDataSource` from a function type defined in
  `apollo-server-env` to a new one defined in `@apollo/utils.fetcher`.
  This function type's return type is compatible with the return type of
  the old fetcher, but its argument types are restricted to a subset of
  the Fetch API that is actually used by these classes (and classes in
  Apollo Server 4). Specifically, they do not allow you to pass
  `Request` or `Headers` objects. Adjust all calls to pass only
  plain JSON objects rather than `Request`/`Headers` objects.

- In `RemoteGraphQLDataSource`, change the type of the "fetch request"
  argument to `parseBody` and `didEncounterError` (which are ignored by
  that class's implementations of the methods but could be examined by a
  subclass's implementations) from `apollo-server-env`'s `Request` type
  to `node-fetch`'s. This `Request` object is actually created at
  runtime by `RemoteGraphQLDataSource` and is always actually a
  `node-fetch` object, so reflecting that in the API seems to be
  reasonable. Note that this object is *not* actually sent to the
  `Fetcher`; it is only sent to these subclass-observable methods. This
  change makes the TypeScript type used for these parameters match the
  actual runtime type precisely so it should be a backwards-compatible
  change.

- In `RemoteGraphQLDataSource`, change the type of the "fetch response"
  argument to `parseBody`, `errorFromResponse`, and `didEncounterError`
  from `apollo-server-env`'s `Response` type to
  `@apollo/utils.fetcher`'s `FetcherResponse` type. This interface type
  was designed to contain all the fields of the `apollo-server-env` type
  so this should be a backwards-compatible change.

- Upgrade `make-fetch-happen` (the default fetcher used to talk to
  Uplink and to subgraphs) from v8 to v10. Stop preventing Renovate from
  upgrading it.

- Changing the `make-fetch-happen` fetcher used in
  `RemoteGraphQLDataSource` to not limit the number of concurrent active
  sockets. (The fetcher used by `UplinkFetcher` keeps its defaults, as
  that fetcher does not need to make concurrent fetches.)

- Remove the exported function `getDefaultFetcher`, as described in the
  CHANGELOG. None of the customizations we were making were actually
  still relevant for its use in `UplinkFetcher` so it seemed simpler to
  just remove the function rather than keep it around purely to allow
  people to simulate the way that an old version of `UplinkFetcher`
  fetched things by default. (We can add it back if there's a lot of
  protest, I suppose, although we'd be unlikely to make it continue to
  export `make-fetch-happen` v8!)

- Replace tests that use Jest mocking of `apollo-server-env` and
  `make-fetch-happen` with `nock` or explicitly passed-in fetchers.
  This simplifies the tests and makes them less tied to the particular
  fetch implementations. (These tests were written before we knew about
  nock.) Also remove some code in IntrospectAndCompose.test.ts that
  *disabled* this mocking so that nock would work.

- Remove the direct dependency on `apollo-server-env`. (Note that some
  types defined in `apollo-server-env` are still used indirectly in
  Gateway, because we still use `apollo-server-types` and types such as
  `GraphQLRequest` contain types from `apollo-server-env`.)

Paired with @trevor-scheer.
trevor-scheer added a commit that referenced this pull request Apr 30, 2022
…(`version-0.x`) (#1810)

* Fix fetch customization, upgrade make-fetch-happen, more concurrency (#1805)

UplinkFetcher and RemoteGraphQLDataSource both use `make-fetch-happen`
to execute HTTP requests and allow you to replace that implementation
with any compatible implementation of the web Fetch API.

But prior to this PR, this customization often did not work. For
example, prior to this PR, the version of `make-fetch-happen` used by
default was the outdated v8. If you wanted to use the current v10, you'd
think you could just install it in your project and pass its `fetch`
function to the appropriate `fetcher` parameter.

But surprisingly, this would not work for `RemoteGraphQLDataSource`! The
Fetch API is flexible and allows you to specify your request either as
plain JSON-style objects or as instances of the `Request` and `Headers`
classes defined in the Fetch API spec. `RemoteGraphQLDataSource` used a
`Headers` object in its requests. Specifically, it used a `Headers`
object imported at runtime from `apollo-server-env`, a package we wrote
that combines custom TypeScript types with `node-fetch` for runtime
behavior.

As it turns out, there's a behavior change in `make-fetch-happen` v9 as
to how it tells the difference between using its API with JSON-style
arguments or the `Requests`/`Headers` class. In the newer version,
`make-fetch-happen` uses an `instanceof` check to see if the provided
object is *its* `Headers` class. So when `RemoteGraphQLDataSource`
passes in the `node-fetch` `Headers` class, `make-fetch-happens` v9 gets
confused (and treats it as an empty JSON-style object instead).

The solution we've settled on is to avoid passing non-plain objects to
any `fetch` call that we intend to be customizable. We've published a
new package, `@apollo/utils.fetcher`, which provides TypeScript
definitions for a subset of the Fetch API that does not include the
dangerous feature of "passing in `Request` or `Headers` objects". This
PR changes both uses of `fetcher` in Gateway to use this new TypeScript
type and to invoke it in a more conservative way. (We're making a
similar change in the upcoming Apollo Server 4.)

This does affect the types passed to some `RemoteGraphQLDataSource`
methods, which are designed to be overridden. We feel reasonably sure
that this won't have any noticeable impact on subclasses, as the changed
types are intended to be very similar to the types they are replacing.

Semi-relatedly, we've changed the default fetcher used by
`RemoteGraphQLDataSource` to allow arbitrarily many parallel connections
to each subgraph rather than only 15. `maxSockets: 15` is the default
for `make-fetch-happen` (though not for the underlying `agentkeepalive`
or `http.Agent` libraries) and so this limit was unintentionally added
to Gateway when we switched from `node-fetch` to `make-fetch-happen`.
While this change is a few years old now, it was unintentional and we
doubt anyone is relying on this; on the contrary, it is causing real
performance problems for production users (#1647). So we are changing
the default to unlimited. If for some reason it is important to you that
your Gateway only be able to process 15 requests at a time, you can
restore the previous behavior:

    import fetcher from 'make-fetch-happen';
    const lowConcurrencyFetcher = fetcher.defaults({ maxSockets: 15 });
    const gateway = new ApolloGateway({
      buildService({ url }) {
        return new RemoteGraphQLDataSource({
          url,
          fetcher: lowConcurrencyFetcher,
        });
      },
    });

Fixes #1647.
Fixes #1287.

In more detail, this change consists of:

- Switch the TypeScript typings `fetcher` option to `new ApolloGateway`
  and `new RemoteGraphQLDataSource` from a function type defined in
  `apollo-server-env` to a new one defined in `@apollo/utils.fetcher`.
  This function type's return type is compatible with the return type of
  the old fetcher, but its argument types are restricted to a subset of
  the Fetch API that is actually used by these classes (and classes in
  Apollo Server 4). Specifically, they do not allow you to pass
  `Request` or `Headers` objects. Adjust all calls to pass only
  plain JSON objects rather than `Request`/`Headers` objects.

- In `RemoteGraphQLDataSource`, change the type of the "fetch request"
  argument to `parseBody` and `didEncounterError` (which are ignored by
  that class's implementations of the methods but could be examined by a
  subclass's implementations) from `apollo-server-env`'s `Request` type
  to `node-fetch`'s. This `Request` object is actually created at
  runtime by `RemoteGraphQLDataSource` and is always actually a
  `node-fetch` object, so reflecting that in the API seems to be
  reasonable. Note that this object is *not* actually sent to the
  `Fetcher`; it is only sent to these subclass-observable methods. This
  change makes the TypeScript type used for these parameters match the
  actual runtime type precisely so it should be a backwards-compatible
  change.

- In `RemoteGraphQLDataSource`, change the type of the "fetch response"
  argument to `parseBody`, `errorFromResponse`, and `didEncounterError`
  from `apollo-server-env`'s `Response` type to
  `@apollo/utils.fetcher`'s `FetcherResponse` type. This interface type
  was designed to contain all the fields of the `apollo-server-env` type
  so this should be a backwards-compatible change.

- Upgrade `make-fetch-happen` (the default fetcher used to talk to
  Uplink and to subgraphs) from v8 to v10. Stop preventing Renovate from
  upgrading it.

- Changing the `make-fetch-happen` fetcher used in
  `RemoteGraphQLDataSource` to not limit the number of concurrent active
  sockets. (The fetcher used by `UplinkFetcher` keeps its defaults, as
  that fetcher does not need to make concurrent fetches.)

- Remove the exported function `getDefaultFetcher`, as described in the
  CHANGELOG. None of the customizations we were making were actually
  still relevant for its use in `UplinkFetcher` so it seemed simpler to
  just remove the function rather than keep it around purely to allow
  people to simulate the way that an old version of `UplinkFetcher`
  fetched things by default. (We can add it back if there's a lot of
  protest, I suppose, although we'd be unlikely to make it continue to
  export `make-fetch-happen` v8!)

- Replace tests that use Jest mocking of `apollo-server-env` and
  `make-fetch-happen` with `nock` or explicitly passed-in fetchers.
  This simplifies the tests and makes them less tied to the particular
  fetch implementations. (These tests were written before we knew about
  nock.) Also remove some code in IntrospectAndCompose.test.ts that
  *disabled* this mocking so that nock would work.

- Remove the direct dependency on `apollo-server-env`. (Note that some
  types defined in `apollo-server-env` are still used indirectly in
  Gateway, because we still use `apollo-server-types` and types such as
  `GraphQLRequest` contain types from `apollo-server-env`.)

Paired with @trevor-scheer.

* Update PR #s

Co-authored-by: David Glasser <glasser@davidglasser.net>
This was referenced May 2, 2022
@glasser
Copy link
Member Author

glasser commented May 7, 2022

While this change is a few years old now, it was unintentional and we doubt anyone is relying on this

Note that this was inaccurate: we moved to make-fetch-happen for GCS/uplink fetching (which isn't particularly concurrent) years ago but only moved to it in RemoteGraphQLDataSource recently in 2.0.0/0.46.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants