Suggestion: Bundling in v17, ESM, CJS, and the dual package hazard

phryneas commented 7 months ago

Submitting here as an issue to reference it in the next WG agenda.

Some historical context puzzle pieces:

deploy full ESM with "type": "module" in "latest-esm" npm tag #3361 painted a way towards a graphql-esm package and a latest-esm tag on the graphql package itself
Switch NPM package to support only ESM #3552 - based on GraphQL-JS-WG decision graphql/graphql-js-wg@main/agendas/2022-03-30.md the approach from #3361 was then adopted for graphql v17.
I am probably missing a bit of history here, because from 17.0.0-alpha.1 to 17.0.0-alpha.2, the CJS<->ESM split seems to have been reintroduced by merging #3361 and going "two packages" again.

There was some discussion in Provide a clear path on how to support graphql-js 16 and 17 at the same time as a esm and cjs library author #3603

Unfortunately, there isn't a lot of documented discussion and I couldn't find any meeting notes for the above graphql-js-wg meeting.

Status Quo

With many tools like Vitest that try to use ESM (but also support CJS), the dual-package hazard still seems to be a real problem. Many packages depending on graphql are still CJS only, so as soon as you try to start in an ESM world (which Vitest does) and import the wrong dependency, you end up in a mix of CJS and ESM, and the instanceof checks break on you.
That makes graphql very hard to use in modern setups.

Proposed solution

I want to suggest a bundling/publishing scheme that @Andarist is successfully using for many packages he maintains, e.g. XState or react-textarea-autosize.

Here's an example entrypoint from https://unpkg.com/browse/xstate@5.11.0/package.json

{
    "main": "dist/xstate.cjs.js",
    "module": "dist/xstate.esm.js",
    ".": {
      "types": {
        "import": "./dist/xstate.cjs.mjs",
        "default": "./dist/xstate.cjs.js"
      },
      "development": {
        "module": "./dist/xstate.development.esm.js",
        "import": "./dist/xstate.development.cjs.mjs",
        "default": "./dist/xstate.development.cjs.js"
      },
      "module": "./dist/xstate.esm.js",
      "import": "./dist/xstate.cjs.mjs",
      "default": "./dist/xstate.cjs.js"
    },
}

Let's zoom in for a second:

{
    ".": {
      "module": "./dist/xstate.esm.js",
      "import": "./dist/xstate.cjs.mjs",
      "default": "./dist/xstate.cjs.js"
    }
}

Here, module would be picked up by bundlers, and import/default would be picked up by runtimes like node.

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 and its contents are:
```
export {
Actor,
// ...snip...
waitFor
} from "./xstate.cjs.js";
```

This ensures that bundlers can pick up a modern ESM build, but in runtime environments like node where the dual-package hazard exists, only one variant of the package exists. With current limitations, that is the CJS build.

I would suggest that we go forward with a packaging scheme like this for v17, as it could solve the dual package hazard, not lock out CJS users and enable gradual adoption of ESM (currently this is very painful and probably holding back a bunch of users from making the switch).
Once ESM is more of an option, v18 could still be ESM only.

From my understanding, the exports field has not been highly adopted when the current decisions around ESM were made, but by now, it is pretty widely supported.

[x] I would volunteer to implement this solution.

PS:

A nice side effect of introducing an exports field would be that we could also use development and production conditions, making a process.env.NODE_ENV check necessary only in a fallback import that would be used if these conditions are not supported by a consumer. That would be follow-up work, though.

kitten commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

Thanks for the clarification @Andarist! :raised_hands:

benjie commented 7 months ago

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

EfstathiadisD commented 5 months ago

Can I ask what is the progress here? Is there a solution being worked on? Do we have some timeline? Or progress with issue? Thanks!

phryneas commented 5 months ago

@EfstathiadisD https://github.com/graphql/graphql-js/pull/4096 and https://github.com/graphql/graphql-js-wg/issues/124#issuecomment-2186584501

graphql / graphql-js