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

Move utility functions out of apollo-env, leaving polyfills. #2274

Merged
merged 4 commits into from
Apr 21, 2021

Conversation

abernix
Copy link
Member

@abernix abernix commented Apr 20, 2021

The apollo-env package has a number of purposes today, including providing polyfills for runtimes that lack more modern features, providing utility functions (like, createHash that works on non-Node.js environments) and offering a common predicate function like isNotNullOrUndefined - a function that we use in many places in the Apollo ecosystem.

The polyfills have proven useful on environments that necessitate them, offering newer ECMAScript functionality in runtimes that don't yet have them, but they're automatically loaded from the main module entry point of this library, rather than being selectively enabled. For example, it's not uncommon to see import 'apollo-env'; in other packages that rely on the functionality.

It's this automatic eager loading of polyfills that proves to be problematic when the utilities and predicate functions of this library come into play though. Imagine a package that merely needs to use a version of createHash that simply falls back gracefully to a JavaScript version, or a function that wants to utilize the useful mapValues function, but in a circumstance where we - or our users - don't want to be subjected to the nature of polyfills affecting the runtime (see related issues like apollographql/apollo-server#2634).

Another circumstance that arises, which was the most motivating factor for me in this case, is that the apollo-env package has consistently proven difficult for bundlers to traverse, possibly because of the polyfills, possibly because of the side-effects, possibly because of both! However, in my times trying to bundle our Apollo packages over the years, I've encountered problems with Webpack, Snowpack, Rollup.js and now esbuild.

Most of these functions are small enough and simple enough that the duplication cost isn't all that high. For example, in my changes in this commit, rather than paying the cost of relying on an other package for what is basically a one-line function, I've just copied and pasted isNotNullOrUndefined into a couple places where it is needed. Furthermore, since most of the utilities that were being leveraged from apollo-env were being leveraged by the apollo-graphql package, I've just moved those into that package instead.

Packages that just need the utilities can rely on apollo-graphql. Any runtimes that still necessitate polyfills like Array.prototype.flat or Array.prototype.flatMap can still import "apollo-env"; directly for those side-effects to be applied to their runtime We can just use core-js-pure to gain the same, well-tested functionality without the overhead of polyfills and the impact those have on consumers/bundlers. Again though, most of our packages are depending on both already so creating the more distinct separation seems not-all-that-unreasonable. Furthermore, while apollo-env does also provide some fetch polyfills which we need to keep, once Node.js 12 becomes the minimum version we support, I don't think we even need non-fetch polyfills anymore at all.

The `apollo-env` package has a number of purposes today, including providing
polyfills for runtimes that lack more modern features, providing utility
functions (like, `createHash` that works on non-Node.js environments) and
offering some common predicate functions like `isNotNullOrUndefined` - a
method that we use in many places in the Apollo ecosystem.

The polyfills have proven useful on environments that necessitate them,
offering newer ECMAScript functionality in runtimes that don't yet have
them, but they're automatically loaded from the main module entry point of
this library, rather than being selectively enabled.  For example, it's not
uncommon to see `import 'apollo-env';` in other packages that rely on the
functionality.

It's this automatic eager loading of polyfills that proves to be problematic
when the _utilities_ and _predicate_ functions of this library come into
play though.  Imagine a package that merely needs to use a version of
`createHash` that simply falls back gracefully to a JavaScript version, or a
function that wants to utilize the useful `mapValues` function, but in a
circumstance where we - or our users - don't want to be subjected to the
nature of polyfills affecting the runtime (see related issues like
apollographql/apollo-server#2634).

Another circumstance that arises, which was the most motivating factor in
this case, is that the `apollo-env` package has consistently proven
difficult for bundlers to traverse, possibly because of the polyfills,
possibly because of the side-effects, possibly because of both!  However,
in my times trying to bundle our Apollo packages over the years, I've
encountered problems with Webpack, Snowpack, Rollup.js and now esbuild.

Most of these functions are small enough and simple enough that the
duplication cost isn't all that high.  For example, in my changes in this
commit, rather than paying the cost of relying on an other package for what
is basically a one-line function, I've just copied and pasted
`isNotNullOrUndefined` into a couple places where it is needed.
Furthermore, since most of the utilities that were being leveraged from
`apollo-env` were being leveraged _by_ the `apollo-graphql` package, I've
just moved those into that package instead.

Any runtimes that still necessitate polyfills like `Array.prototype.flat` or
`Array.prototype.flatMap` can still `import "apollo-env";` directly for
those side-effects to be applied to their runtme, and packages that just
need the utilities can rely on `apollo-graphql`.  Again though, most of
our packages are depending on both already so creating the more distinct
separation seems not-all-that-unreasonable.  Furthermore, while `apollo-env`
does also provide some `fetch` polyfills, once Node.js 12 becomes the minimum
version we support, I don't think we even need those polyfills anymore at all.
@abernix abernix force-pushed the abernix/apollo-env-utilities-to-elsewhere branch from 6d2f69b to 9207475 Compare April 20, 2021 10:59
…forms

Honestly, this whole attempt to use modern ECMAScript features before they
were ready and truly supported in the runtimes we support was I think a
great experiment, but not worth the struggle.  We still support Node.js 10
for a couple more weeks, but this is a constraint for me today and it's
(being, generally the `apollo-server-env` and the `apollo-env` package's
polyfills) have been a constraint _many_ times over.

Put another way, I don't think the Pandora's box of "polyfills" is worth
it for things like `Array.protytype.flat`, which are not so inherently
complex that they can't be abstracted into a small function.

The `core-js-pure` package does a pretty good job offering this
functionality without necessitating us in-house/vendor it ourselves, but it
- sadly - does not provide TypeScript types, unlike the `core-js` core
itself.
@abernix abernix force-pushed the abernix/apollo-env-utilities-to-elsewhere branch from d817b56 to cc82776 Compare April 20, 2021 13:39
Comment on lines 29 to 30
// TODO(Node.js 10): When we deprecate Node.js 10, remove this and switch
// to using `Array.prototype.flat`.
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
// TODO(Node.js 10): When we deprecate Node.js 10, remove this and switch
// to using `Array.prototype.flat`.
// TODO(Node.js 10): When we deprecate Node.js 10, remove this and switch
// to using `Array.prototype.flat`. When doing this, deleting the hand-rolled
// types in `./packages/apollo-gateway/src/types/` that go with it.

@abernix abernix merged commit b1bd747 into master Apr 21, 2021
@abernix abernix deleted the abernix/apollo-env-utilities-to-elsewhere branch April 21, 2021 07:10
trevor-scheer added a commit to trevor-scheer/apollo-tooling that referenced this pull request Apr 26, 2021
Related to apollographql#2274, which removes a dependency on apollo-env which
transitively required this package (presumably, unconfirmed but I
will validate).

This also adds typings to the require() statements for the
respective modules.
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.

1 participant