esm entrypoint with named exports

[giltayar]

Description of the Change

Now that Mocha supports ESM test files, I found that importing Mocha in ESM files is awkward, if you're like me and don't like to rely on the it/before/... globals. So, if you used to do this in CJS:

const {it, before} = require('mocha')

You'd like to do this in ESM:

import {it, before}  from 'mocha'

But you can't, because Mocha 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 mocha  from 'mocha'
const {it, before} = mocha

This PR exports an ESM entry point so that you can use named exports in a natural manner when importing Mocha 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).

Alternate Designs

There is no alternate design, as this is the only way to do what are called dual-mode packages in ESM.

Why should this be in core?

Well, where else can it be? ☺️

Benefits

Importing mocha in a manner that is natural for ESM.

Possible Drawbacks

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.

But this has not been tested, as Mocha doesn't have an E2E test that checks that the package is OK. The way to write this test is to do an npm pack that creates a tar.gz, and then create a package that has as a dependency that tar.gz, and then test whatever we want in there. This ensures that the package as a package (without the original source code) is OK. I did not add this test, as it feels way out of scope of this little addition.

Applicable issues

[boneskull]

ty @giltayar.

I started fiddling with smoke-test even before I saw this PR... should probably get it done. there are other efforts in the works too by various individuals in the community

[boneskull]

We do have test/smoke, which we should be able to use in the meantime. I'll give that a try.

[boneskull]

@giltayar in the future, you can create branches in the mochajs/mocha repo; just prefix them with giltayar/, e.g., giltayar/node-esm-support. this will give us better test coverage, because the full test suite (on saucelabs) cannot run for PRs alone

[boneskull]

@giltayar I've added the missing index.mjs to the files prop of package.json ;)

rewrote the smoke tests to install mocha from the output of npm pack

[boneskull]

and rebased onto master

[boneskull]

@giltayar does "./" allow package.json as well? I want to be very sure about this.

[coveralls]

Coverage increased (+0.05%) to 94.191% when pulling 44e2a06 on giltayar:node-esm-support into c667d10 on mochajs:master.

[giltayar]

ty @giltayar.

I started fiddling with smoke-test even before I saw this PR... should probably get it done. there are other efforts in the works too by various individuals in the community

What you call "smoke test", I call E2E tests. Oh, well: testing terminology has always been a bit vague.

In any case, my personal preference is for all tests to be part of the npm test test suite, and to never have tests that run in CI (unless absolutely necessary, like Windows-only tests).

But anyway, thanks for adding the ESM smoke test and the missing index.mjs!

[giltayar]

@giltayar does "./" allow package.json as well? I want to be very sure about this.

Yes. I just checked it locally on a dummy module, and it works (it allows require-ing the package.json of a package). I knew it would, but fear of another is-promise made me double-check 😀.

[ljharb]

You can use npx ls-exports package mocha@latest and compare to npx ls-exports path . run inside this PR's working directory, and make sure you're not accidentally creating a breaking change.

[giltayar]

I tried @ljharb's utility and got some strange results. So I played around with require-ing mocha and seeing what it really requires, and found out something: require-ing mocha when running regular code is not the same as require-ing mocha when running under Mocha.

When running under Mocha, you get additional properties (before, after...) which you get only when running under Mocha. Not sure why they aren't exported in both cases.

What should be done is to add both of them to the index.mjs: both the additional "dynamic" properties (which I have already in the index.mjs), and the "static" properties (which I'm going to add right now).

[boneskull]

yeah running npm-pack-style tests locally would be good. was easier to hack it out in a shell script for now

[ljharb]

@giltayar if you find anything broken, please do file an issue :-)

[boneskull]

@giltayar errrr... hmm. upon further consideration, there are problems here:

• Mocha tests are not "node-able". You cannot run node test.js where test.js imports/requires mocha and expect it to work like it would w/ mocha. you must use bin/mocha or must otherwise manually bootstrap it.
• describe, it, etc., are put into the global object dynamically. furthermore, they are dynamically injected into module.exports of the main package entry point! this is because different interfaces (bdd being the default, but there's tdd, exports, qunit, etc.) use different APIs.
• which interface is exported is not determined until Mocha is fully bootstrapped. meaning require('mocha') in a test loaded with the qunit interface will have a different API than one loaded with the bdd interface!
• this works similarly, but even more weirdly, in a browser (module.exports = global, anyone?)

I think we should carefully consider how we want to export APIs for ESM instead of trying to mimic what happens in CJS.

Keep in mind, this is new code, so we're not going to break anyone.

But, IMO, describe, it, beforeEach, etc., should not be statically exported as we have here.

Alternative A

// mytest.spec.mjs
import {describe, beforeEach, it} from 'mocha/bdd';
import {suite, suiteSetup, test} from 'mocha/tdd';
// etc

with the documented caveat that you must run your code via bin/mocha for this to work.

Alternative B

Make tests work via node test.spec.mjs. Again, we have the opportunity to do this because it's new and won't break existing users. I am not sure what that implementation will look like, but it will necessarily not be as feature-rich as running with bin/mocha. Personally, I don't really know if this is worth the bother. People do want this (we've had an open issue for many years), but it feels more like tossing a bone to the "don't use globals, ever" crowd instead of providing something that's widely useful.

Alternative C

Don't export anything other than what lib/mocha.js stuffs into module.exports before bootstrapping (no interface APIs). If someone wants to consume mocha via ESM, they can use the provided API to do their own bootstrapping. This will break the "ESM-style smoke test" I've added.

Thoughts? @nicojs @Munter @craigtaub @outsideris

[boneskull]

Thanks @coreyfarrell and @ljharb for looking at this

[boneskull]

Mocha is a great example of both the advanced flexibility and wanton abuse of Node.js' CJS module system.

[giltayar]

There is Option D:

export all interfaces, and let the user choose whichever one they want, which is basically what they do today. I'm not sure how Mocha decides which ones to really export (I always used describe/it/...), but maybe we can just export all of them? While not the optimal solution, I'm not sure there is an optimal solution, and given that the interface to the module is dynamic, and we have to "staticize" it, I believe this to be the simplest.

Moreover, it'll probably be the least surprising to users, who got used to doing const {...} = require('mocha'). They would be surprised that they need to do something other than import {...} from 'mocha'.

[craigtaub]

I agree D will be the least surprising to users. But my preference is A as it introduces more clarity between the interfaces (both require the mocha executable).

Also agree regarding B, having interface as point-of-entry bootstrapping the parsing + runner execution phases sounds like introducing more complexity, I feel we should only do this route if we see real value.

[giltayar]

I feel like there's either A or D. I personally would prefer D, because I like simple and unsurprising, but of course, whatever will be chosen, I will implement.

My preference for D is also thinking about the future, where we'll be getting questions (StackOverflow and such) about why isn't describe exported from ESM Mocha, whereas it was exported in CJS. Now that I understand, I can definitely explain, but (i) I'll probably have to do the explaining again and again, and (ii) not everybody will have (or take) the time to understand.

[craigtaub]

why isn't describe exported from ESM Mocha, whereas it was exported in CJS

Think thats a fair point. To counter my own argument, there aren't that many interfaces to muddle up, so do not think any added clarity is worth future confusion. Im happy with either A or D.

[boneskull]

To be very clear, if you want D:

• Not all interfaces are exported when using require('mocha'); only the interface you have chosen (by default that's bdd, with describe, etc.). This change would not retain "parity", but rather go beyond what commonjs does.
• If D is implemented, all methods of all interfaces will be exported, but the only ones that will work are those from the chosen interface. What the others will do is unknown to me, but certainly one of a) throw an exception, b) silently fail, c) something else.

I thought of another way. It would not require us to export things that are broken, however.

Alternative E

// or some other name
import {currentInterface} from 'mocha';

// in bdd mode
const {describe, it} = currentInterface();

// in tdd mode
const {suite, test} = currentInterface();

Functions can be dynamic. ESM exports can't.

[ljharb]

@giltayar yes, you can use the deprecated slash patterns to make it not be semver-major - i was assuming the preference would be to avoid deprecated features.

[juergba]

subpath_folder_mappings is the one being deprecated.
subpath_patterns is the experimental one to be used instead.

also available in V14: subpath_patterns

[giltayar]

I finally had the time to look at this. This is a real bug, but not necessarily in this PR. It uncovered a bug in ESM combined with --parallel.

On a machine with more than one CPU, run node bin/mocha test/integration/fixtures/esm/*.fixture.mjs --no-color --no-bail --parallel (in this branch!). It will run three tests in two fails, as expected.

Now, run the same, but with an additional --jobs 1. You will get a Error [ERR_REQUIRE_ESM]: Must use import to load ES Module, because it's not going through the async branch of Mocha.

It's something around the connection between parallel and "lazyLoadFiles", but I haven't tracked it yet.

My guess is that fixing this bug will also fix this branch's bug. Looking into it...

[giltayar]

The tests here fail due to a real bug: #4559

[giltayar]

Finally! All tests have passed, and this PR also fixes #4559.

[juergba]

@giltayar I'm trying to understand this PR, but haven't got it yet.
Why do you insert package-lock.json into this PR?

[giltayar]

@juergba From what I see in the commit, I just modified the file that was already there, which I'm assuming my npm install did after I changed package.json (I only added an exports section without touching the dependencies in package.json). Did I do anything wrong here?

[juergba]

I just added main and exports fields to my package.json. There are no changes to package-lock.json after npm install.

We have had quite a few dependencies updates in recent weeks. It helps to delete your node_modules folder periodically before executing npm install.

[ljharb]

When no dep changes are intended, any lockfile changes that appear should be discarded.

[giltayar]

@juergba It's because I used npm v7 (we switched to it at work about a month ago).

Sp I reverted to the upstream package-lock.json, then npm install-ed, and pushed.

This made package-lock.json move out of this PR.

Nice catch! And sorry for the confusion.

[juergba]

@giltayar I haven't digested the complete matter, but my first impressions is, I don't really like it.
The starting point was, that below code is awkward:

import mocha  from 'mocha'
const {it, before} = mocha

and we want (?) this: import {it, before} from 'mocha'

This PR goes too far and at the same time stops on half way.
Too far:

• IMO big effort/risk for small added value. I know you are an ESM maniac, but ... 😎
• do we really need parity between CJS and ESM, and then - even more complex - backwards compability for CJS?
• the fallback "./": "./" has gone
• I haven't understood the proxy part. Does it belong into this PR?

stop on half way:

• we have more interfaces
• what are the consequences for third party interfaces?
• you export unloadFile (which makes no sense for ESM)
  • what about the rest of our public functions?
  • what about private functions which are used by other packages (eg. collectFiles (?))

[giltayar]

@juergba thanks for the input. Here are my responses:

IMO big effort/risk for small added value. I know you are an ESM maniac, but ... 😎

I am! Hopefully, it's a compliment 😉. What I'm seeing in the ecosystem is more and more packages adding ESM wrappers (or, like SIndre Sorhus' packages, migrating all in into ESM). So is it a small value? Last year, maybe. This year, I don't think so. It's slowly becoming a baseline requirement.

do we really need parity between CJS and ESM, and then - even more complex - backwards compability for CJS?

Not sure I understand what you mean.

the fallback "./": "./" has gone

That's easy. I can easily put it back. There was input here that maybe we should go all in, and do it as a semver major that abolishes all "illegal" entrypoints into mocha. But if the sentiment is not to do it, I can very easily restore it.

I haven't understood the proxy part. Does it belong into this PR?

Unfortunately, yes. When using parallel execution, and the number of workers is less than the number of test files, a worker can be reused for two test files. This exposes the fact that the describe/it/... that are required/imported are actually tied to the first Mocha instance that uses them, and when a new Mocha instance is created, they ignore it and use the previous one. To deal with that, I had to proxy all those functions so that they always find the "current" Mocha instance.

we have more interfaces

Ouch. I must have missed those. If you tell me what those are, I can easily add them.

what are the consequences for third party interfaces?

I have no idea. I didn't know Mocha has this capability. Where can I find documentation (or an example of such an interface) so that I can try and figure out the implications?

you export unloadFile (which makes no sense for ESM)

Good catch. I'll remove it from the ESM exports.

what about the rest of our public functions?

I researched carefully, and this is what I found. I may have missed a spot. Which ones did I miss?
what about private functions which are used by other packages (eg. collectFiles (?))

what about private functions which are used by other packages (eg. collectFiles (?))

I could export them, but they are easily accessible (I think) as properties of the default export, no? I think that we should leave it like that. But if you think otherwise, tell me which, and I'll export them "legally".

[juergba]

refering third party interfaces: see here in our wiki

[giltayar]

@juergba Thanks for the pointer. As far as I can tell, the third party ui interface functions will hang the interface functions on global. This will continue to work with no problem. This PR didn't change anything around that.

If they wish to have an entry point similar to what Mocha exports via this PR, then we can't do that for them, and they will need to do supply one on their own (outside of Mocha). This is because if we'd wanted to supply one, we'd need to:

1. Understand what interface functions they're supplying. This is currently not supported by the method that the Wiki supplies.
2. Even if we added a way for the third party ui to supply the interface functions, we can't dynamically generate an entry point with the appropriate exports, given that in ESM export MUST be static. We could do that for CJS, but I'm not sure it's worth the effort, although I may be wrong and third party UIs are widely used.

BTW, the proxy part of this PR has been superseded by a much cleaner method to do the proxying in #4574 (thanks @nicojs!). Once that PR is merged, I will merge my PR and remove all the proxy code that won't be needed anymore.

[github-actions]

This PR hasn't had any recent activity, and I'm labeling it stale. Remove the label or comment or this PR will be closed in 14 days. Thanks for contributing to Mocha!