Distribute ESM alongside CJS

[trevor-scheer]

Issue for tracking ESM support. PRs welcome! @apollo/server distributes both, so that could provide some inspiration for the work necessary to implement this.

[laverdet]

Why not just distribute ESM (instead of only CJS, which is what we're doing now). It's been 6 years since ESM support was added to nodejs, the ecosystem has moved on and you're creating more work for yourself. node-fetch dropped CJS entirely quite a while ago.

Edit: I bumped into this issue today because we're unable to upgrade to @apollo/server 4.9.4 from 4.7.4 since @as-integrations/koa imports Apollo as CommonJS and we import it as ESM. It looks like there's an explicit annotation in @apollo/server's types now which aims to prevent the dual package hazard.

[trevor-scheer]

Unfortunately supporting both (in the Apollo Server project) is a requirement for now. This package does not have to follow that requirement since this is a community project in theory (and in practice once someone other than myself wants to maintain it) but as long as I'm maintaining it my preference would be to ship both. It might be a bit more work, but it's work we've done before, should be able to replicate fairly easily, and we only have to do it once.

Alternatively, we can fork this repo and ship a new ESM-only package if that's something you'd be interested in maintaining. I'd be equally happy with that outcome.

Apollo's reality is that we support customers who use CJS, so while plenty of popular packages have made the decision to move on, that's not an option for us yet.

[laverdet]

It might be a bit more work, but it's work we've done before, should be able to replicate fairly easily, and we only have to do it once.

It is work that's been done before incorrectly. Right now, today, Apollo Server suffers from the dual package hazard. Instances of a server created from ESM will be similar but unrelated to instances created in CJS. This will lead to subtle bugs where instanceof checks fail mysteriously, or global state is maintained in 2 separate locations.

We can observe this in the CLI:

// incorrect dual exports
> require('@apollo/server').ApolloServer === (await import('@apollo/server')).ApolloServer
false
// does not suffer dual package hazard
> require('node:fs').readFile === (await import('node:fs')).readFile
true

To correctly support dual exports is actually pretty annoying with lots of tradeoffs. The incorrect support for dual package exports in @apollo/server is what precipitated my comment here. If support in that package was implemented correctly then I would probably have never noticed that this package is CommonJS. To copy and paste an incorrect implementation only poisons the ecosystem further.

[trevor-scheer]

Fair point, I'll be sure to get it addressed in AS first before I duplicate anything that's incorrect and propagate the issue.
apollographql/apollo-server#7625

In the meantime, forking it yourself or creating an ESM-only repo to live in this org are my best options for you.

[laverdet]

In the meantime, forking it yourself or creating an ESM-only repo to live in this org are my best options for you.

Yeah definitely. It's a single file so I think we're just going to copy/paste into our repository. Thanks!