Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Gateway: introduce
make-fetch-happen
#3783Gateway: introduce
make-fetch-happen
#3783Changes from 7 commits
6dfb96f
59a5697
517966c
1c1ce29
d5c52bc
efeae34
bbe3dee
a537651
7ac184c
40acac8
535fb10
5b3a333
2b3f234
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need to implement a
delete
method? When does it get called?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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#--optscachemanagerOn that note, I'm unsure if I should export the
CacheManager
interface from the gateway package or if I can leave it in themake-fetch-happen
definition file. Thoughts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trevor-scheer I think we should leave it in
make-fetch-happen
, and avoid exposing this directly to the user, for now.This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
fetch
thatmake-fetch-happen
still accepts aRequest
(in place ofstring
for the first argument), but also options passed as the second argument (known as "init", by the specification which are represented by theRequestInit
type. That type lives on thedom
lib in TypeScript (warning, large file), but we intentionally have a rough-copy of it in ourapollo-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 inRequestInit
are extended with those supported bymake-fetch-happen
— represented (but I haven't validated the completeness of it) byFetcherOptions
above.I think we might want something like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a code readability standpoint, I chose to use the types themselves and effectively dupe the
fetch
type here: 2b3f234I 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 inapollo-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 haveapollo-env
andapollo-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.