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 Node.js ESM entry point with named and default exports #1340

Merged
merged 2 commits into from
Jun 9, 2020

Conversation

giltayar
Copy link

Now that Mocha supports ESM test files, I found that importig Chai in ESM files is awkward. So, if we used to do this in CJS:

const {expect} = require('chai')

You'd like to do this in ESM:

import {expect}  from 'chai'

But you can't, because Chai doesn't have an ESM entry point, and relies on the fact that CJS files can be imported, but only using default import. So you have to do this:

import chai  from 'chai'
const {chai} = chai

This PR exports an ESM entry point so that you can use named exports in a natural manner when importing Chai itself.

The way it does this is to add an exports section to the package.json, as described in the Node.js documentation for exports. This exports has a "conditional export" that defines separate entry points for ESM and CJS.

Note that exports affects both ESM and CJS, so we should be careful (see ramifications below): based on the case of is-promise, theoretically this can break anybody using Mocha, because adding exports: ... to package.json affects CJS too! It means that the only entrypoints allowed (in CJS and ESM) are the ones defined in the exports .

Which is why I added "./": "./" to the exports, which says to Node.js: allow any deep importing such as require("mocha/foo/bar") in the case that somebody, somewhere, is using that. This was also the solution used by is-promise.

@giltayar giltayar requested a review from a team as a code owner May 23, 2020 04:14
@sebamarynissen
Copy link

Hi @giltayar,

I've already created something similar a while ago (#1317). I didn't really study the is-promise case in depth. Do you think my solution is is-promise proof?

@giltayar
Copy link
Author

@sebamarynissen your solution is emphatically not is-promise problem proof. DO NOT MERGE PR #1317. If you do, you will bar people from deep-importing into chai, even when requiring. This is because when you add an exports, then the entry points defined by that are the only entry points to the module, both in ESM and CJS.

This is why in this PR I added the line:

"./": "./"

In the exports: so that deep linking will be enabled.

This PR also adds testing this feature, which was not trivial given that it can only be tested in Node versions that support ESM.

@giltayar
Copy link
Author

The better way to do this is to define the documented entry points in the exports section and remove the "./": "./" fallback, so that only official entry points can be used, and I can easily do that, but if it was up to me, I would wait for a semver-major upgrade in order to do that, so as not to upset people that use unofficial entry points.

@sebamarynissen
Copy link

Yes, that was what I was thinking of as well now: wait for a semver major and leave it up to the core team to decide what entry points to export.

@giltayar
Copy link
Author

@sebamarynissen - not necessarily. You can publish this PR now with the "./": "./" fallback (and thus support unofficial entry points), and in the next semver-major we remove that fallback and add the official entry points.

Copy link
Member

@lucasfcosta lucasfcosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it'd be fully backwards compatible I don't see any problems with having this in.

@keithamus keithamus merged commit e08ca08 into chaijs:master Jun 9, 2020
@sebamarynissen
Copy link

Just for information, are there plans to release a new version on npm with this feature anytime soon? Given that Node 14 becomes LTS in October I think that being able to use named imports with chai will speed up the adoption of ES modules on Node.

chrisveness added a commit to chrisveness/geodesy that referenced this pull request Dec 14, 2021
ESM import now canonical: github.com/chaijs/chai/pull/1340.
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.

4 participants