Suggestion: Bundling in v17, ESM, CJS, and the dual package hazard about graphql-js

[kitten]

Just to give my 2c, I'm not sure if exports.development and exports.production target conditionsa are widely supported, so some import.env shenanigans may still be necessary until that's widely adopted.

On the topic of this, I find this section a lot more interesting and that's often been on my mind:

you end up in a mix of CJS and ESM, and the instanceof checks break on you.

In my opinion, with graphql-js’ unique situation of being a reference standard implementation, it's rather unfortunate that the schema-parts of this library are limited to reference equality when it comes to inheritance chains and checks, and I'd love to see this part be simply removed and replaced. (Be that with static constants, symbol checks, typed Symbol.toStringTag checks, etc)

This might of course force some level of API-intercompatibility with graphql itself across versions, but that's already a reality that exists for the AST, which sees much more re-use and adoption.

imho the problem of ESM and CJS exports being both imported and then intercompatible, which can sometimes happen in Node, is an optimisation-opportunity/issue that's to be fixed and discovered by users, but it's highly inconvenient that the instanceof checks break in these scenarios.
Fixing the ESM/CJS bundling issues doesn't get rid of the underlying problem of the instanceof checks imho. So no matter what the outcome here is, I'd love to see that addressed too, if everyone's in favour

[phryneas]

Just to give my 2c, I'm not sure if exports.development and exports.production target conditionsa are widely supported, so some import.env shenanigans may still be necessary until that's widely adopted.

More modern build chains (e.g. vite) would pick them up. It won't be an immediate solution, more of a long-term hope ;)
But it was more of a "PS" here, definitely something to tackle a bit later.

Fixing the ESM/CJS bundling issues doesn't get rid of the underlying problem of the instanceof checks imho.

I totally agree, and instanceof could be something that could be tackled independently. A lot of times, ASTs are serialized and deserialized, and after that it might be hard or impossible for graphql-js to work with them.
That said, I'd say we choose that battle for another day - even if instanceof were eliminated in this package, the dual-package hazard might still cause problems, e.g. if downstream packages used instanceof, or just by pulling packages into the bundle twice, resulting in massive bundle size increases.

I'd like to address the CJS <-> ESM problem first.

[benjie]

Can you detail why ./dist/xstate.cjs.mjs is even needed? My understanding is that it simply imports from the CommonJS code, which end-user code could do in the same way? The end result being that we only need to expose common JS and it should just work with Node ESM import { GraphQLInt } from 'graphql' statements anyway?

[yaacovCR]

• xstate.esm.js is an ESM module. It will be picked up by bundlers for both import and require - they don't care.
• xstate.cjs.js is a CommonJS module as you might expect - this would be picked up by require calls in node.
• xstate.cjs.mjs is the actually interesting one. It's ESM, but it only re-exports the CommonJS module. It will be picked up in node for import calls

overall this looks really well thought out and it's great that it apparently is in use successfully in other packages!

one question I have is whether it is a problem for any non-node, non-bundler environments that the "import" condition points to esm that imports cjs rather than just esm. Is there an esm-only environment that this might block?

Not asking from a place of knowledge, but just something I noticed.

Asking from a different direction => if we have an esm only that reimports cjs, why do we need the pure esm version? which environments does that help specifcally, and are they all covered by "module"?

[Andarist]

Can you detail why ./dist/xstate.cjs.mjs is even needed?

Because if you write:

export const foo = 42

You don't expect this to be a valid import:

import _ns from 'pkg'
pkg.foo

This file effectively "normalizes" the shape of the module back to what was authored.

one question I have is whether it is a problem for any non-node, non-bundler environments that the "import" condition points to esm that imports cjs rather than just esm. Is there an esm-only environment that this might block?

I haven't found any - given the node's popularity everyone is trying to be compatible with it and handle CJS. If absolutely needed you could always fight it back with some more specific conditions put above it (like if u'd have to target Deno then u could use the deno condition etc)

Asking from a different direction => if we have an esm only that reimports cjs, why do we need the pure esm version? which environments does that help specifcally, and are they all covered by "module"?

That pure ESM version is mainly targeting bundlers so they can benefit from tree-shaking and other optimizations that are often only implemented for ESM files.

Most bundlers automatically use module condition - if they don't have that logic enabled by default... well, they should still work since they should behave like node. They just won't benefit from those optimizations.

[benjie]

Thanks for the clarification @Andarist! 🙌

[benjie]

Some interesting reading on the impact of this on TypeScript users (which I believe is already addressed in the solution above, albeit in a slightly different way to this article): https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseESM.md