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

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

Merged
merged 5 commits into from
Jun 30, 2020

Conversation

abernix
Copy link
Member

@abernix abernix commented Jun 29, 2020

This is a similar treatment as to what was applied to @apollo/gateway in #3783.

It replaces a "fetcher" implementation we'd been maintaining (which continues to grow in complexity) which was built on node-fetch. Instead, it switches to an off-the-shelf Fetch API-compliant implementation called make-fetch-happen which leverages minipass-fetch 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, 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 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 and uses an in-memory store. We only have a finite set of artifacts to store from the registry that generally only represent a smallish number of kilobytes, so an in-memory store should be suitable. If something more is desired, we offer the ability to provide a custom fetcher directly to the ApolloGateway constructor options!

If there was a suitable place to depend on this cache 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 the cache implementation 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.

abernix added 4 commits June 29, 2020 17:20
This is to make sure they are available to both `@apollo/gateway` (which
already uses them) and, soon (in an upcoming commit), the
`apollo-server-plugin-operation-registry` plugin.
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
The Fetch API uses `GET` by default, so this should never be necessary.
@abernix abernix requested a review from trevor-scheer June 29, 2020 19:18
@abernix abernix changed the title op-reg: Switch from node-fetch to make-fetch-happen. chore(op-reg): Switch from node-fetch to make-fetch-happen. Jun 29, 2020
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.

LGTM!

* We are attempting to get types included natively in this package, but it
* has not happened, yet!
*
* See https://github.com/npm/make-fetch-happen/issues/20
Copy link
Member

Choose a reason for hiding this comment

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

🧐 @-me looks like it might be time to follow up on that one!

@abernix abernix merged commit 0830b66 into main Jun 30, 2020
@abernix abernix deleted the abernix/op-reg-change-fetcher-impl branch June 30, 2020 10:09
trevor-scheer added a commit that referenced this pull request Jun 30, 2020
The hand-written make-fetch-happen types are required to exist within
the packages that use them in order for them to build correctly when
installed within another project.

This reverts the portion of #4326 which moves these types to the top
level of the project and duplicates them across the two packages which
depend on them: apollo-gateway and apollo-server-plugin-operation-registry.
trevor-scheer added a commit that referenced this pull request Jun 30, 2020
The hand-written make-fetch-happen types are required to exist within
the packages that use them in order for them to build correctly when
installed within another project.

This reverts the portion of #4326 which moves these types to the top
level of the project and duplicates them across the two packages which
depend on them: apollo-gateway and apollo-server-plugin-operation-registry.
abernix added a commit to apollographql/federation that referenced this pull request Sep 4, 2020
…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
abernix pushed a commit to apollographql/federation that referenced this pull request Sep 4, 2020
…llo-server#4333)

The hand-written make-fetch-happen types are required to exist within
the packages that use them in order for them to build correctly when
installed within another project.

This reverts the portion of apollographql/apollo-server#4326 which moves these types to the top
level of the project and duplicates them across the two packages which
depend on them: apollo-gateway and apollo-server-plugin-operation-registry.
Apollo-Orig-Commit-AS: apollographql/apollo-server@97c3acd
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants