Skip to content

Commit

Permalink
chore(op-reg): Switch from node-fetch to make-fetch-happen. (apol…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
abernix authored Jun 30, 2020
1 parent 01ebac9 commit 2e0c8c9
Showing 1 changed file with 0 additions and 53 deletions.
53 changes: 0 additions & 53 deletions gateway-js/src/make-fetch-happen.d.ts

This file was deleted.

0 comments on commit 2e0c8c9

Please sign in to comment.