Gateway: introduce `make-fetch-happen` #3783

trevor-scheer · 2020-02-12T07:02:53Z

This PR introduces the make-fetch-happen package into the gateway. In this initial implementation, gateway uses this library for fetches to GCS only.

This package is effectively a drop-in replacement for our usages of node-fetch and removes the need for the cachedFetcher which we wrote and maintain ourselves (whose logic is also shared by apollo-server-plugin-operation-registry).

Additionally, (to the main point) this package offers various caching options, retry functionality, and proxy support just to name a few of the features that we're looking to support with the gateway. Overall, this seems like a great first step towards making these features accessible to our users.

~~TODO: types for the library seem to be non-existent. I wasn't meticulous with the type definitions I created - they seem sufficient but should probably be vetted.~~

Obligatory

packages/apollo-gateway/src/snapshotSerializers/queryPlanSerializer.ts

packages/apollo-gateway/src/cache.ts

abernix · 2020-02-19T12:28:52Z

packages/apollo-gateway/src/cache.ts

+  async delete(request: Request) {
+    const key = cacheKey(request);
+    const entry = await this.cache.get(key);
+    await this.cache.delete(key);
+    return Boolean(entry);
+  }


Do we actually need to implement a delete method? When does it get called?

Ours is never actually called (at least within our tests), however I do want our exported interface to comply with make-fetch-happen's requirements listed here: https://github.com/npm/make-fetch-happen#--optscachemanager

On that note, I'm unsure if I should export the CacheManager interface from the gateway package or if I can leave it in the make-fetch-happen definition file. Thoughts?

@trevor-scheer I think we should leave it in make-fetch-happen, and avoid exposing this directly to the user, for now.

packages/apollo-gateway/src/make-fetch-happen.d.ts

packages/apollo-gateway/src/cache.ts

packages/apollo-gateway/src/index.ts

abernix · 2020-02-19T16:12:24Z

packages/apollo-gateway/src/make-fetch-happen.d.ts

+    match(req: Request): Promise<Response | undefined>;
+  }
+  export interface Fetcher {
+    (url: string): Promise<Response>;


The fetch that make-fetch-happen still accepts a Request (in place of string for the first argument), but also options passed as the second argument (known as "init", by the specification which are represented by the RequestInit type. That type lives on the dom lib in TypeScript (warning, large file), but we intentionally have a rough-copy of it in our apollo-server-env:

https://github.com/apollographql/apollo-server/blob/bbe3dee5bce50f39459b92192edf3b58b89125fb/packages/apollo-server-env/dist/fetch.d.ts#L47-L71

Additionally, I think that the default options that fetch accepts in RequestInit are extended with those supported by make-fetch-happen — represented (but I haven't validated the completeness of it) by FetcherOptions above.

I think we might want something like this:

import { fetch as envFetch } from 'apollo-server-env'; export interface Fetcher { ( input?: Parameters<typeof envFetch>[0], init?: Parameters<typeof envFetch>[1] & FetcherOptions, ): ReturnType<typeof envFetch>; }

From a code readability standpoint, I chose to use the types themselves and effectively dupe the fetch type here: 2b3f234

I won't deny that your approach couples the types and some potential maintenance burden of duplicated types. Interested to know your thoughts, since I'm pretty torn, tbh.

I guess my perspective is exactly that. In other words, I suggested this because I don't really care to keep myself up to date on what new options the Fetch API eventually introduces; there was a new draft of the specification that landed in January. The, e.g., node-fetch package is likely going to stay on top of that, and I'm glad they will.

However, my suggestion here doesn't solve an existing problem — we already maintain our own Fetch types in apollo-server-env. 😬 More than anything, I wish that were not the case right now and my biggest concern is that we continue in that direction — we already have apollo-env and apollo-server-env in some of the @apollo/* packages.

Let's certainly ship this how it is for now, but just keep our eyes on it.

packages/apollo-gateway/src/make-fetch-happen.d.ts

abernix · 2020-02-19T17:45:58Z

For the failing Node.js 6 tests, I think we can bypass those tests for Apollo Gateway since we never officially supported it (it came out after Node.js 6 fell out of LTS.).

Feel free to do that however feels necessary — one option is passing parameters to the commands in the CircleCI configuration, and another is the approach — which is the common technique right now but might not be sufficient or reasonable — which we use for some tests:

apollo-server/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts

Line 485 in bbe3dee

(NODE_MAJOR_VERSION === 6 ? describe.skip : describe)(

Follows-up: #3783 Follows-up: #3829

* docs: Document new `fetcher` on `@apollo/gateway`. Follows-up: #3783 Follows-up: #3829 * Update apollo-gateway.mdx * Update docs/source/api/apollo-gateway.mdx Co-Authored-By: Trevor Scheer <[email protected]> Co-authored-by: Trevor Scheer <[email protected]>

This is a similar treatment as to what was applied to `@apollo/gateway` in @trevor-sheer's #3783. This replaces a local HTTP cache implementation which continues to grow in complexity with an off-the-shelf Fetch API client with many valuable bells and whistles. In particular, it uses `make-fetch-happen`, which is a relatively full-featured Node.js based implementation which is used by `npm` itself and leverages `minipass-fetch` under the hood. This leverages the same cache implementation used in `@apollo/gateway` and duplicates that cache logic. It's entirely possible we would be well-served to use the `cache.js` example that's included in the source of `make-fetch-happen` and leverages `cacache`, but as of this message, it doesn't include the TypeScript types and this implementation seems to work. Ref: https://npm.im/make-fetch-happen Ref: https://npm.im/minipass-fetch Ref: https://npm.im/cacache Ref: https://github.com/npm/make-fetch-happen/blob/b04c4c16/cache.js

This is a similar treatment as to what was applied to `@apollo/gateway` in #3783. This replaces a local HTTP cache implementation which continues to grow in complexity with an off-the-shelf Fetch API client with many valuable bells and whistles. In particular, it uses `make-fetch-happen`, which is a relatively full-featured Node.js based implementation which is used by `npm` itself and leverages `minipass-fetch` under the hood. This leverages the same cache implementation used in `@apollo/gateway` and duplicates that cache logic. It's entirely possible we would be well-served to use the `cache.js` example that's included in the source of `make-fetch-happen` and leverages `cacache`, but as of this message, it doesn't include the TypeScript types and this implementation seems to work. Ref: https://npm.im/make-fetch-happen Ref: https://npm.im/minipass-fetch Ref: https://npm.im/cacache Ref: https://github.com/npm/make-fetch-happen/blob/b04c4c16/cache.js

This is a similar treatment as to what was applied to `@apollo/gateway` in #3783. This replaces a local HTTP cache implementation which continues to grow in complexity with an off-the-shelf Fetch API client with many valuable bells and whistles. In particular, it uses `make-fetch-happen`, which is a relatively full-featured Node.js based implementation which is used by `npm` itself and leverages `minipass-fetch` under the hood. This leverages the same cache implementation used in `@apollo/gateway` and duplicates that cache logic. It's entirely possible we would be well-served to use the `cache.js` example that's included in the source of `make-fetch-happen` and leverages `cacache`, but as of this message, it doesn't include the TypeScript types and this implementation seems to work. This does remove a test which was previously valuable but should no longer be necessary. Specifically, sine we now have an HTTP implementation that handles caching and retries itself, we do handle intermediary retries within that layer. We still test the polling (i.e, the literal existence of an interval which re-fires) in other tests, but it was too tricky to try to re-jigger this test with the abstraction. I think this is a good thing to not need to worry about, but we can consider re-adding it in the event of regressions. Ref: https://npm.im/make-fetch-happen Ref: https://npm.im/minipass-fetch Ref: https://npm.im/cacache Ref: https://github.com/npm/make-fetch-happen/blob/b04c4c16/cache.js

This is a similar treatment as to what was applied to `@apollo/gateway` in #3783. This replaces a "fetcher" implementation we'd been maintaining (which continues to grow in complexity) which was built on [`node-fetch`][1]. Instead, it switches to an off-the-shelf [Fetch API][2]-compliant implementation called [`make-fetch-happen`][3] which leverages [`minipass-fetch`][4] under the hood. It's also what `npm` uses internally! Whereas `node-fetch` is relatively bare-bones and necessitated us writing our own [conditional request][5], `make-fetch-happen` just does these things (and also other things, like supporting `HTTP_PROXY` out of the box!). It does, however, need us to provide a cache store since we cannot use its (default) file-system based cache (which leverages the powerful [`cacache`][6] implementation used by `npm`) since file-systems are not available in all of our supported integrations. Therefore, this duplicates the same cache implementation used in `@apollo/gateway`. If there was a suitable place to depend on this code from both of these packages, we could depend on this implementation from that location, but I didn't immediately see a great place for that to live. Also, rule of threes or something? (Also worth noting that it _already includes_ the fix which I opened in #4325 / 2f29c60 which needed to be applied to the gateway implementation.) This does REMOVE A TEST which was previously valuable but should no longer be nearly as valuable. Specifically, since we now have an HTTP implementation that handles conditional requests, a cache, and retries itself, we do handle intermediary retries within that layer. We still test the polling (i.e, the literal existence of an interval which re-fires) in other tests, but it was too tricky to try to re-jigger this test with the abstraction. I think this is a good thing to not need to worry about, but we can consider re-adding it in the event of regressions. [1]: https://npm.im/node-fetch [2]: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API [3]: https://npm.im/make-fetch-happen [4]: https://npm.im/minipass-fetch [5]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Conditional_requests [6]: https://npm.im/cacache Do not attempt to cache requests with no response body. A `HEAD` request has no body to cache and a 304 response could have only been negotiated by using a cached body that was still valid. Therefore, we do NOT write to the cache in either of these cases. Without avoiding this, we will invalidate the cache, thus causing subsequent conditional requests (e.g., `If-None-Match: "MD%") to be lacking content to conditionally request against and necessitating a full request/response.

This is a similar treatment as to what was applied to `@apollo/gateway` in #3783. This replaces a "fetcher" implementation we'd been maintaining (which continues to grow in complexity) which was built on [`node-fetch`][1]. Instead, it switches to an off-the-shelf [Fetch API][2]-compliant implementation called [`make-fetch-happen`][3] which leverages [`minipass-fetch`][4] under the hood. It's also what `npm` uses internally! Whereas `node-fetch` is relatively bare-bones and necessitated us writing our own [conditional request][5], `make-fetch-happen` just does these things (and also other things, like supporting `HTTP_PROXY` out of the box!). It does, however, need us to provide a cache store since we cannot use its (default) file-system based cache (which leverages the powerful [`cacache`][6] implementation used by `npm`) since file-systems are not available in all of our supported integrations. Therefore, this duplicates the same cache implementation used in `@apollo/gateway`. If there was a suitable place to depend on this code from both of these packages, we could depend on this implementation from that location, but I didn't immediately see a great place for that to live. Also, rule of threes or something? (Also worth noting that it _already includes_ the fix which I opened in #4325 / 2f29c60 which needed to be applied to the gateway implementation.) This does REMOVE A TEST which was previously valuable but should no longer be nearly as valuable. Specifically, since we now have an HTTP implementation that handles conditional requests, a cache, and retries itself, we do handle intermediary retries within that layer. We still test the polling (i.e, the literal existence of an interval which re-fires) in other tests, but it was too tricky to try to re-jigger this test with the abstraction. I think this is a good thing to not need to worry about, but we can consider re-adding it in the event of regressions. [1]: https://npm.im/node-fetch [2]: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API [3]: https://npm.im/make-fetch-happen [4]: https://npm.im/minipass-fetch [5]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Conditional_requests [6]: https://npm.im/cacache

) * Introduce `make-fetch-happen` for GCS requests * Implement in memory cache manager for make-fetch-happen * Provide local typings for `make-fetch-happen` * Skip node v6 tests for federation and gateway packages Apollo-Orig-Commit-AS: apollographql/apollo-server@9420aa0

…lographql/apollo-server#4326) This is a similar treatment as to what was applied to `@apollo/gateway` in apollographql/apollo-server#3783. This replaces a "fetcher" implementation we'd been maintaining (which continues to grow in complexity) which was built on [`node-fetch`][1]. Instead, it switches to an off-the-shelf [Fetch API][2]-compliant implementation called [`make-fetch-happen`][3] which leverages [`minipass-fetch`][4] under the hood. It's also what `npm` uses internally! Whereas `node-fetch` is relatively bare-bones and necessitated us writing our own [conditional request][5], `make-fetch-happen` just does these things (and also other things, like supporting `HTTP_PROXY` out of the box!). It does, however, need us to provide a cache store since we cannot use its (default) file-system based cache (which leverages the powerful [`cacache`][6] implementation used by `npm`) since file-systems are not available in all of our supported integrations. Therefore, this duplicates the same cache implementation used in `@apollo/gateway`. If there was a suitable place to depend on this code from both of these packages, we could depend on this implementation from that location, but I didn't immediately see a great place for that to live. Also, rule of threes or something? (Also worth noting that it _already includes_ the fix which I opened in apollographql/apollo-server#4325 / 5b2d2d89e4ca6 which needed to be applied to the gateway implementation.) This does REMOVE A TEST which was previously valuable but should no longer be nearly as valuable. Specifically, since we now have an HTTP implementation that handles conditional requests, a cache, and retries itself, we do handle intermediary retries within that layer. We still test the polling (i.e, the literal existence of an interval which re-fires) in other tests, but it was too tricky to try to re-jigger this test with the abstraction. I think this is a good thing to not need to worry about, but we can consider re-adding it in the event of regressions. [1]: https://npm.im/node-fetch [2]: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API [3]: https://npm.im/make-fetch-happen [4]: https://npm.im/minipass-fetch [5]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Conditional_requests [6]: https://npm.im/cacache Apollo-Orig-Commit-AS: apollographql/apollo-server@0830b66

trevor-scheer changed the title ~~Initial implementation of make-fetch-happen~~ Gateway: introduce make-fetch-happen Feb 12, 2020

trevor-scheer requested a review from abernix February 12, 2020 07:10

trevor-scheer changed the base branch from master to release-2.11.0 February 18, 2020 19:50

trevor-scheer added 2 commits February 18, 2020 11:52

Initial implementation of make-fetch-happen

6dfb96f

Implement in memory cache manager for make-fetch-happen

59a5697

trevor-scheer force-pushed the trevor/make-fetch-happen branch from d5dba30 to 59a5697 Compare February 18, 2020 19:53

Cleanup types

517966c

trevor-scheer marked this pull request as ready for review February 18, 2020 21:44

trevor-scheer commented Feb 18, 2020

View reviewed changes

packages/apollo-gateway/src/snapshotSerializers/queryPlanSerializer.ts Outdated Show resolved Hide resolved

trevor-scheer added 3 commits February 18, 2020 17:20

Cleanup

1c1ce29

Improve provided typings for make-fetch-happen

d5c52bc

Add @ts-ignore with explanation

efeae34

trevor-scheer force-pushed the trevor/make-fetch-happen branch from e77895d to efeae34 Compare February 19, 2020 01:26

Add changelog entry

bbe3dee

trevor-scheer commented Feb 19, 2020

View reviewed changes

packages/apollo-gateway/src/cache.ts Outdated Show resolved Hide resolved

abernix reviewed Feb 19, 2020

View reviewed changes

trevor-scheer added 6 commits February 19, 2020 11:38

Skip node v6 for federation and gateway packages

a537651

Rename Cache -> HttpRequestCache

7ac184c

Simplify cache key generation

40acac8

Cache native objects rather than stringified versions of them

535fb10

Use the FetcherOptions type

5b3a333

Fixes to handwritten make-fetch-happen types

2b3f234

trevor-scheer merged commit 9420aa0 into release-2.11.0 Feb 20, 2020

trevor-scheer deleted the trevor/make-fetch-happen branch February 20, 2020 00:34

trevor-scheer mentioned this pull request Feb 20, 2020

[FEATURE] Open to TypeScript type definitions? npm/make-fetch-happen#20

Closed

abernix added this to the Release 2.11.0 milestone Feb 25, 2020

abernix mentioned this pull request Feb 25, 2020

Switch to existing fetch type, rather than that of make-fetch-happen. #3829

Merged

abernix added a commit that referenced this pull request Mar 12, 2020

docs: Document new fetcher on @apollo/gateway.

27dee37

Follows-up: #3783 Follows-up: #3829

abernix mentioned this pull request Mar 12, 2020

docs: Document new fetcher on @apollo/gateway. #3886

Merged

abernix mentioned this pull request Jun 29, 2020

chore(op-reg): Switch from node-fetch to make-fetch-happen. #4326

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gateway: introduce `make-fetch-happen` #3783

Gateway: introduce `make-fetch-happen` #3783

trevor-scheer commented Feb 12, 2020 •

edited

Loading

abernix Feb 19, 2020

trevor-scheer Feb 19, 2020 •

edited

Loading

abernix Feb 20, 2020

abernix Feb 19, 2020

trevor-scheer Feb 20, 2020

abernix Feb 20, 2020

abernix commented Feb 19, 2020

Gateway: introduce make-fetch-happen #3783

Gateway: introduce make-fetch-happen #3783

Conversation

trevor-scheer commented Feb 12, 2020 • edited Loading

abernix Feb 19, 2020

Choose a reason for hiding this comment

trevor-scheer Feb 19, 2020 • edited Loading

Choose a reason for hiding this comment

abernix Feb 20, 2020

Choose a reason for hiding this comment

abernix Feb 19, 2020

Choose a reason for hiding this comment

trevor-scheer Feb 20, 2020

Choose a reason for hiding this comment

abernix Feb 20, 2020

Choose a reason for hiding this comment

abernix commented Feb 19, 2020

Gateway: introduce `make-fetch-happen` #3783

Gateway: introduce `make-fetch-happen` #3783

trevor-scheer commented Feb 12, 2020 •

edited

Loading

trevor-scheer Feb 19, 2020 •

edited

Loading