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

Add runtime dependency on node-fetch #1970

Merged
merged 5 commits into from
Jul 18, 2022
Merged

Conversation

benweatherman
Copy link
Contributor

@benweatherman benweatherman commented Jul 9, 2022

We use node-fetch during runtime for some of the public methods in RemoteGraphQLDataSource. Using node-fetch@2 because v3 is ESM-only. There's already a renovate rule to keep things from going to v3.

We should probably break the interfaces that are using node-fetch's classes, since they aren't used by the default implementation after switching to make-fetch-happen.

Other detritus

Fixes #1961

TODO

  • Investigate using plain-ol-javascript-object headers instead of Headers object
  • Add CHANGELOG entry

We do use `node-fetch` during runtime for some of the public methods in `RemoteGraphQLDataSource`. Using `node-fetch@2` because v3 is ESM-only. There's already a renovate rule to keep things from going to v3.

We should probably break the interfaces that are using `node-fetch`'s classes, since they aren't used by the default implementation after switching to `make-fetch-happen`.

### Other detritus

- **Move `@types/node-fetch` to `devDependencies`** This was originally included in apollographql/apollo-server#3546 as a fix for apollographql/apollo-server#3471. I think this is more appropriate for types.
- **Change some imports to use `type`** so there's no runtime dependency for things that are just using types.
- **Add `@types/make-fetch-happen`**
- **Remove `pretty-format`** since that's not a runtime thing

Fixes #1961
@netlify
Copy link

netlify bot commented Jul 9, 2022

Deploy Preview for apollo-federation-docs canceled.

Name Link
🔨 Latest commit cf1046d
🔍 Latest deploy log https://app.netlify.com/sites/apollo-federation-docs/deploys/62d5e29073a04a0008a8ae7f

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 9, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@benweatherman benweatherman added this to the 2.1.0 milestone Jul 9, 2022
@benweatherman benweatherman marked this pull request as ready for review July 9, 2022 11:56
That doesn't make a ton of sense to do
Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Agree the dependency appears to be required currently because we literally use new Headers to pass in the headers to the fetch implementation.

That said, am curious if we could opt to just pass a plain object as headers rather than a Headers. It's my understanding that most fetch implementations will accept merely an object, and the Fetch API specification suggests that it's completely fine to do so. Is that worth a consideration?

Anyhow, just a thought. LGTM, but also like the idea of not including the dependency at all if we can avoid it.

@benweatherman
Copy link
Contributor Author

curious if we could opt to just pass a plain object as headers

I like this line of thinking and I tried but got hung up on some type conflicts that were buried a bit too far. Lemme dig those up so we can talk specifics, instead of this hand-wavy response I've got so far.

@benweatherman
Copy link
Contributor Author

appears to be required currently because we literally use new Headers to pass in the headers

I missed this part of the comment earlier. We also use new NodeFetchRequest to pass to some callbacks like thus (so I think we'd have to break that interface to start supporting a more generic Request, since NodeFetch has things like agent, etc)

// Note that we don't actually send this Request object to the fetcher; it
// is merely sent to methods on this object that might be overridden by users.
// We are careful to only send data to the overridable fetcher function that uses
// plain JS objects --- some fetch implementations don't know how to handle
// Request or Headers objects created by other fetch implementations.
const fetchRequest = new NodeFetchRequest(http.url, requestInit);
So even if we get rid of new Headers, we'll still have that interface to support.

That said...

We use this type declaration for headers for all of our GraphQLRequests, which provides a lot more methods than a plain-ol-javascript-object. https://github.com/apollographql/apollo-server/blob/3e0b6f917588052d179811fac366889056b34073/packages/apollo-server-env/src/fetch.d.ts#L13-L26. So we'd have to break the GraphQLRequest interface to support plain-ol-javascript-objects.

TL;DR

Removing node-fetch would require several underlying changes, and I'm too lazy to do that right now.

@benweatherman benweatherman enabled auto-merge (squash) July 18, 2022 22:46
@benweatherman benweatherman merged commit 0fd83f0 into main Jul 18, 2022
@benweatherman benweatherman deleted the weatherman/runtime-node-fetch branch July 18, 2022 22:50
clenfest added a commit that referenced this pull request Aug 30, 2022
#1970 seems to have caused a regression based on `@types/node-fetch` not being in the gateway's runtime dependencies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node-fetch runtime dependency is missing in @apollo/gateway v2.0.5
2 participants