Different dists for CJS and ESM are causing both to be used, leading to TypeScript problems

[ephys]

Issue Description

@apollo/server provides different builds for ESM and CJS. It is great to see more ESM support, but the current approach means it suffers from the Dual package hazard described in the node documentation.

To sum up the hazard: this approach makes it extremely likely that both the ESM and CJS versions will be loaded at the same time. Here is a simple example of how this can happen:

• Your project is written in ESM.
• Your project uses @nestjs/apollo. nestjs is written in commonJS and imports @apollo/server
• Your project includes a plugin for apollo, so you also import @apollo/server
• Your project will load the ESM version of apollo, and nest will load the CJS version. Meaning the plugin cannot be compatible with nest.

I strongly recommend doing everything you can to only ship a single build to node users by either:

• Dropping support for CJS altogether
• Or using the "ES module wrapper" approach. The first approach described here: https://nodejs.org/api/packages.html#dual-commonjses-module-packages. Approach 2 suggested by node does not work for TypeScript users. Here is a real world example and a test that makes sure the wrapper is kept in sync

I've provided a minimalist example of what sort of errors this causes below

Link to Reproduction

https://codesandbox.io/p/sandbox/laughing-sun-85cmy8?file=%2Fsrc%2Findex.ts%3A1%2C1

Reproduction Steps

See above

[trevor-scheer]

Thanks for taking the time to explain, demonstrate, and recommend options. I can't ask for a better reproduction than that. Admittedly, we've tried to spend the bare minimum amount of time worrying about this and dropping CJS isn't an option for us yet.

Just to probe for understanding - if @nestjs/apollo were shipping ESM/both, this issue wouldn't manifest since we'd be loading ESM in both cases?

From what I gathered in the sequelize monorepo, it looks like the build script is generating that es module wrapper via esbuild? Somewhat related: another contributor recently showed me this to help us get our dual typings correct, and it looks like they aren't quite right for the sequelize repo. Is that something you're already aware of? Is it possible to simultaneously publish dual typings and take the wrapper approach?

I'm not sure if/when I'll be able to prioritize working on this. I'll leave this open in case someone feels motivated to fix it though, and I'd be more than happy to review a PR.

[ephys]

if @nestjs/apollo were shipping ESM/both, this issue wouldn't manifest since we'd be loading ESM in both cases?

There would still be two (very unlikely) cases where both versions could be loaded:

• Because dynamic import() is available to both CJS and ESM, if a user had a CJS codebase but used dynamic import() to load @nestjs/apollo.
• On the flip side, if someone has an ESM codebase but loads @nestjs/apollo using createRequire

Somewhat related: another contributor recently showed me this to help us get our dual typings correct, and it looks like they aren't quite right for the sequelize repo. Is that something you're already aware of? Is it possible to simultaneously publish dual typings and take the wrapper approach?

Absolutely. We actually received the same report recently (https://github.com/sequelize/sequelize/issues/16157). Thankfully the fix is simple, we made the mistake of using the same typing file for both CJS and ESM. They can have the same content but they must have different file extensions.

From what I gathered in the sequelize monorepo, it looks like the build script is generating that es module wrapper via esbuild?

We are writing our codebase in ESM, but we use esbuild to transpile it to CJS, and we have a small ESM wrapper file (that is not transpiled) that defines the ESM named exports

I'll leave this open in case someone feels motivated to fix it though, and I'd be more than happy to review a PR.

I'd be happy to open a PR to fix this. I think I'll fix https://github.com/sequelize/sequelize/issues/16157 first to make sure we have a viable solution, then bring a similar setup over

[trevor-scheer]

That would be lovely, thank you! Happy to review and validate that to the best of my ability.

[trevor-scheer]

Hey @ephys, any update here? I've got another report of this being problematic so I'm going to try to get this prioritized. Just want to check with you before duplicating efforts. TIA!

[trevor-scheer]

tsup seems like a worthwhile exploration into tooling which claims to solve this (possibly in combination with https://turbo.build/repo/docs/handbook/publishing-packages/bundling - the guide here uses both together)

[d3c0d3dpt]

tsup seems like a worthwhile exploration into tooling which claims to solve this (possibly in combination with https://turbo.build/repo/docs/handbook/publishing-packages/bundling - the guide here uses both together)

tsup on that example still provides both CJS & ESM outputs, which would still result on that dual package hazard.

I would be happy to help here, but I'm unsure of what are the next steps. @trevor-scheer any thoughts?

[trevor-scheer]

@d3c0d3dpt I can't really advise on next steps, but the explanation @ephys provided sounds like an end state I'd be happy with. If I were trying to do this I would probably strip down the build system and iterate on it with the end state they proposed in mind:

We are writing our codebase in ESM, but we use esbuild to transpile it to CJS, and we have a small ESM wrapper file (that is not transpiled) that defines the ESM named exports

I would also expect tests similar to our existing smoke tests to validate the changes work as expected and also prevent the dual package hazard.

[d3c0d3dpt]

I see a problem related to using a ESM wrapper - did some tests on wrapping a CJS module, and it impacts the tree-shaking efficiency that a bundler (like esbuild) has.

As far as I understand, this relates to the fact that CommonJS require of a module will bring everything on it's entry point to the bundle, regardless of it all being used or not.

Is this something you would be ok with?

[d3c0d3dpt]

I see a problem related to using a ESM wrapper - did some tests on wrapping a CJS module, and it impacts the tree-shaking efficiency that a bundler (like esbuild) has.

As far as I understand, this relates to the fact that CommonJS require of a module will bring everything on it's entry point to the bundle, regardless of it all being used or not.

Is this something you would be ok with?

@trevor-scheer / @ephys any thoughts on this?

[trevor-scheer]

@d3c0d3dpt I'd be curious to know how tree-shakeable Apollo Server actually is in order to form an opinion. Would you be able to try bundling Apollo Server as ESM and CJS in its current form to see how much it currently benefits from that?

I know that serverless users generally prefer the smallest bundle possible due to cold start time, so I don't want to be insensitive to that.

[d3c0d3dpt]

Hey @trevor-scheer

So, I've managed to to build an example for that scenario - see it here.

Despite it looking like the resulting bundle size is the almost the same ("pure" ESM is 57kb bigger than "wrapped" ESM), this is due to a dual package hazard caused by @apollo/utils* being published as CJS - it causes a double import of @apollo/usage-reporting-protobuf and lru-cache.

Were these packages to be published as ESM, it would be possible to tree-shake at least 240kb of additional code.

From looking at the meta files using this tool, I can see that there @graphql-tools footprint was reduced by 150kb, and it is the main difference. @apollo/server difference would be around 37kb (@apollo/server 147kb vs @apollo/server-wrapped 184kb).

Let me know if I can provide some other example :)

Edit 1 - attached wrapped-esm-meta.json and pure-esm-meta.json which can be viewed on the Bundle Size Analyzer.

[rsteele1-seic]

Hi All! Sorry to cross-post but as I reported in (https://github.com/apollo-server-integrations/apollo-server-integration-aws-lambda/pull/152#issuecomment-1989575238) this is totally blocking us from upgrading from v3. Will the v3 deprecation be extended if this isn't resolved soon? Has progress been made on this elsewhere and I just missed a ticket? Thanks for the excellent work!

[ephys]

I see a problem related to using a ESM wrapper

Ah yep, that's unfortunate. Considering the alternative, I would say the ESM wrapper is still the better approach but that apollo-server should consider moving to being an ESM-only module so the tree-shaking ability is safely guaranteed

Also, very sorry for the delay. I just now pushed a PR to the Sequelize repo to fix the last outstanding issues with the wrapper. I'll try to do the same in this one