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

apollo-server-env: do less #5515

Merged
merged 4 commits into from
Jul 20, 2021
Merged

apollo-server-env: do less #5515

merged 4 commits into from
Jul 20, 2021

Conversation

glasser
Copy link
Member

@glasser glasser commented Jul 20, 2021

apollo-server-env is already less useful in Apollo Server 3 because we don't try to support Node 6 and we don't use the global type definitions (https://www.apollographql.com/docs/apollo-server/migration/#apollo-server-envs-global-type-definitions). Turns out we can remove even more from it without much ill effect.

@glasser glasser requested a review from abernix July 20, 2021 04:57
@glasser
Copy link
Member Author

glasser commented Jul 20, 2021

@abernix Thoughts?

After this PR, apollo-server-env is basically just a wrapper around node-fetch and url (plus an index.browser.js file to ostensibly support some browser environment). I'm half tempted to just switch all uses of apollo-server-env to node-fetch and url and pretend that index.browser.js never existed, though probably there's a real use case supported there (Cloudflare, perhaps, based on git history?).

@glasser
Copy link
Member Author

glasser commented Jul 20, 2021

And for example, see #5514 which tries to make a change to the Request type... to make it more like node-fetch. Yeah, increasingly convinced that after merging this PR we should just stop using apollo-server-env.

One thing that might make this PR or the potential follow-up PR awkward is that @apollo/gateway also depends on apollo-server-env. Arguably, this is another argument for killing the package and just making everything depend on node-fetch, so that weird changes to this package don't accidentally break gateway later...

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.

Changes LGTM.

No strong feelings on the direction you take after this though, but I'm personally somewhat satisfied with requiring a bundler take on the burden of providing a global Fetch-API and WHATWG URL.

We might want to understand precisely what that burden looks like for, e.g., Deno and Cloudflare though, sure. (And probably document it?)

@glasser
Copy link
Member Author

glasser commented Jul 20, 2021

Deno is an interesting point.

I wonder if there is a way to keep the package separate for providing JS implementation, but not have to maintain our own copy of the TypeScript though. I tried half-heartedly to delete all the .d.ts and just re-export directly from node-fetch and url, and it ran into a bit of trouble (basically around the places where the .d.ts differs from the DefinitelyTyped types, which arguably are bugs in our types...). Might try harder later, might not.

Node 6 support meant we needed to polyfill util.promisify,
Object.values, and Object.entries. But we now only support Node 12+.

Fixes #5478.
These are both exported from apollo-server-types as well, and almost all
of the usages use that package (which doesn't have the special build
script setup that apollo-server-env has).
I'm not really sure why this was needed before (probably for Node 6
support?) but tests pass without it now.
@glasser glasser enabled auto-merge July 20, 2021 16:03
@glasser glasser merged commit 33c6e13 into main Jul 20, 2021
@glasser glasser deleted the glasser/fewer-polyfills branch July 20, 2021 16:06
@abernix
Copy link
Member

abernix commented Jul 20, 2021

I thought our types were intentionally a subset of what node-fetch has and closer to what the Fetch API standard itself offers.

That was meant to offer compatibility with other fetch implementations like make-fetch-happen, potential for those that might be bundling for the browser to use the native fetch, and still played nicely with node-fetch which has features that extend the WHATWG/W3C specified behaviour.

Using the types from the TypeScript dom lib seems ideal tbh, but it's apparently not straightforward to merely package that fetch-y subset of the larger lib.

@hwillson hwillson removed this from the MM-2021-07 milestone Jul 29, 2021
@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.

3 participants