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 (sequelize/sequelize#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 sequelize/sequelize#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?