Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add exports field to support Node.js ESM #3217

Closed
wants to merge 2 commits into from

Conversation

PabloSzx
Copy link

@PabloSzx PabloSzx commented Jul 28, 2021

Recently I've authored a blog post that works as a guide for enabling ESM Support for Node.js libraries: What does it take to support Node.js ESM? that might help in giving some context, fortunately, this package has most of the work already done 🎉 , but there is an import missing piece.

The exports field adds support for Node.js ESM through Conditional exports, which is required to be able to use the already existing ".mjs" files.

Since Node.js v12.20.0 (2020-11-24) and v14.13.0 (2020-09-29) the latest and finally stable version of package.exports is available, and since support for Node.js v10.x is already dropped, everything should be fine.

This change can already be tested with a simple package that installs "graphql", and adding the "exports" field manually in the "package.json" of the graphql package.

Adding this field for the major version "v16" is the perfect moment to do it, since, for people that are currently using ESM with the "graphql" package, all the imports were using the ".js" CommonJS version. With this change, if another library that uses graphql as peer dependency doesn't support ESM, it will use the CommonJS version, and the Node.js application is going to have a duplicated "graphql" instance, and the current state of this library doesn't support a duplicated instance of itself.

You might think that the previously mentioned issue is a bad thing, but I think it's a good thing since it will push the Node.js ESM Ecosystem forward, something that is very needed currently.

Nothing changes for people only using CommonJS

There is nothing to fear for people using only CommonJS, this change doesn't affect anything in that regard, and this change just enables compatibility for both "environments", which is one of the main reasons for the existence of Node.js Conditional exports in the first place

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 28, 2021

CLA Signed

The committers are authorized under a signed CLA.

@PabloSzx
Copy link
Author

v16 already released, and since adding exports field is a breaking change it shouldn't be considered until v17

@saihaj
Copy link
Member

saihaj commented Oct 28, 2021

It was touched once in WG but in next release if we to we can drop support for cjs. Up for discussion

@PabloSzx
Copy link
Author

PabloSzx commented Oct 28, 2021

we can drop support for cjs. Up for discussion

Converting graphql-js into full ESM with type: module will cause tons of conflicts, personally, all my current projects use ESM so I wouldn't have an issue, but that is not the case for the majority for the people, for example, Jest support for ESM is horrendous, and you can't even use dynamic imports in CJS with the jest VM module context, see node-fetch/node-fetch#1263 nodejs/node#36351 nodejs/node#33216 jestjs/jest#11438

@IvanGoncharov
Copy link
Member

@PabloSzx The problem here is that we can't have two copies of graphql-js in one program.
Meaning if you have an ESM project but some of your dependencies use graphql-js as CJS.
You will fail instanceof checks.

Node.js team suggests having ESM files be wrappers against CJS and it will work and resolve problem with duplicating versions.
However, if ESM files will be wrappers it breaks all use cases that require real ESM files (e.g. native loading, see #2277) also it's bad for tree-shaking and can cause other issues with bundlers.

I'm happy to implement any solution that will have native ESM files and wouldn't cause instanceof conflicts if users have both ESM and CJS in their dependencies.

@PabloSzx
Copy link
Author

PabloSzx commented Nov 1, 2021

Thinking about it more in-depth the duplicated graphql-js truly is a big problem that is even harder to debug and diagnose than doing a "type" : "module" approach, It should be completely possible to release another version of graphql-js just like experimental-stream-defer but for ESM with type module, but still having exports field to support extensionless imports, and people that are already full ESM (and every library that they use support it) can simply do:

(pnpm | yarn | npm) add graphql@esm
{
  "graphql": "16.0.0-esm.0"
}

@PabloSzx
Copy link
Author

PabloSzx commented Nov 4, 2021

Created #3361 creating a new build script based on the current one but doing a "type": "module" with complete "exports" fields, also published https://npm.im/graphql-esm which used this exact script, used it in an example repo that uses only libraries that have full support for ESM, everything works perfectly https://github.com/PabloSzx/test-graphql-esm

@PabloSzx
Copy link
Author

PabloSzx commented Nov 4, 2021

Closing in favor of #3361

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants