chore(infra): Stop mocking fetch modules, use nock instead
#1287
Comments
glasser
added a commit
that referenced
this issue
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.
glasser
added a commit
that referenced
this issue
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
pushed a commit
that referenced
this issue
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 issue
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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We currently mock out the modules for
node-fetchandmake-fetch-happenand leverage that in some areas but not others. This adds a bit more magic than one might expect (myself, a person who's spent plenty of time working in this repo) and isn't well-documented or even a convention we've coalesced on.nockis something that we already use and is very explicit in test code with no unexpected mocking of modules happening. To make this change, we'd simply replace all calls tomockResponseOnceandmockJSONResponseOncewith equivalent usages ofnockand delete thenode-fetchandmake-fetch-happenmocks altogether.The text was updated successfully, but these errors were encountered: