Node esm support

[giltayar]

Description of the Change

This PR adds support for Node.js's native ESM (ES modules) support.
Specifically, up till now, Mocha could only load test files that were CommonJS,
and this PR enables it to load test files that are ES modules,
and thus enable the test files to import stuff from other ES modules
(not to mention other goodies coming up the line, like enabling top-level await, which is an ESM only JS feature).

Note that this support is for Node.js only, and has no connection to browser ESM support.
Also, prior to Node v12, there was no or quirky support for ESM, so this support is only for Node v12 and above.

Alternate Designs

This is not a trivial change. The main problem is that loading an ESM is an async operation, and yet
the call path to Mocha.prototype.loadFiles is a sync one. Changing the call path would necessitate a
semver-major change, which I'm not sure I have the ability to push, so I decided on a design
that keeps the signatures of all the functions as they are, unless there is an ESM test file to load, by which
time they morph to async functions. So their return type in the signature looks
something like Runnner|Promise<Runner>, where Runner is for cases where there are no ESM test files,
and Promise<Runner> is for the case there are

This is not a good solution, but it enables us to push this change without it being semver-major, and once
there is a decision to change to semver-major, the signature change and code change that does this is trivial.

If y'all decide that this PR can change the major version of Mocha, I can fix it so that the signatures
change and the weirdness goes away.

Another not insignificant change was the error path. In some cases, errors found by Mocha were synchronous, i.e.
they occur during the yargs.parse phase, which throws the error. But, because the call path to loadFiles could
be asynchronous, I changed the command handler (in lib/cli/run.js) to be async (i.e to return a Promise).
This means that errors (such as bad tap versions) are no more synchronous, and will always arrive at the
fail yargs handler (in lib/cli.js). To make them look like the old errors, I changed the code
there. No tests fail now, so it seems OK, but please check this in the review.

Why should this be in core?

I'm assuming this is obvious: it has to change Mocha.prototype.loadFiles to be async. There is the esm module that simulates ESM in userland, but native support has to be async. And native support is crucial if the developer's code is running using Node's native ESM support.

Benefits

Developers starting to write Node.js code in ES modules will benefit by being able to write their tests using
ESM too, and not have to dynamically import their ES modules using dynamic import (aka await import), which is a hassle and a kludge.

Possible Drawbacks

This may impact code using Mocha's API, as we are changing the signature (even if not in the short run,
definitely in the long run). Also, error handling has subtly changed, and this may impact error handling reporting
in ways I cannot know.

Applicable issues

This is currently an enhancement (minor release), that should be turned in the future into a semver-major,
as described in the "Alternate Designs" section.

Limitations

• "Watch" mode does not currently work with ESM, as there is currently no way to remove an ESM module from Node.js' module cache. We would have to either wait for it to be implemented in Node (slim chance, IMHO), or refactor watch mode to use subprocesses every time a change is detected.
• Module-level mocks via libs like proxyquire or rewiremock or rewire cannot work. If you are using these libs or similar, hold off on using ESM for your test files.. This is not inherently a limitation in Mocha, but rather a limitation in Node's ESM support, which will probably be rectified sometime in Node v13.
• ESM is not supported for custom reporters, nor is it supported for custom interfaces.

[jsf-clabot]

All committers have signed the CLA.

[coveralls]

Coverage increased (+0.02%) to 92.9% when pulling e1728f8 on giltayar:node-esm-support into a995e33 on mochajs:master.

[giltayar]

I fixed the bug in Windows (it was actually a bug in the code, but it only manifested itself in Windows).

Unfortunately, there is a bug in Windows support for ESM. The third ESM test I wrote fails in Windows in Node v12.6.0 (the version Appveyor runs), but passes in Node v12.11.1 (the latest Node version I downloaded today).

I'm not sure how to upgrade the Node version in Appveyor, so I can disable this test for Node versions < 12.11.1?

[giltayar]

It seems the bug in Node ESM implementation is not just in Windows, but also in Linux, so I disabled ESM support in Mocha for Node <v12.11.0.

[juergba]

This is not a good solution, but it enables us to push this change without it being semver-major, and once there is a decision to change to semver-major, [...]

We should make this decision at first place. Why should we try to avoid semver-major?
The user - while using Mocha programmatically - would have to load the files manually, right?

var mocha = new Mocha();
mocha.addFile();
mocha.loadFiles();
mocha.run();

Would the user be affected in any other breaking way by a semver-major?
I strongly tend to vote for semver-major in order to avoid the changes Mocha.prototype.run().

PS: Could you please edit your description above and add the require.cache / watch restrictions?

[giltayar]

@juergba today loadFiles is internal and part of run. To understand what will change, let's look at your code (modified to remove loadFiles):

var mocha = new Mocha();
mocha.addFile();
mocha.run();
console.log('Done!)

If we broke the interface, it would have to look something like this:

var mocha = new Mocha();
mocha.addFile();
mocha.run().then(() => console.log('done'));

Because Mocha.run will now return a Promise<Runner>, and not a Runner. That means that if this code is in a function, that function, instead of being sync, and returning whatever, it will have to be async and return Promise<whatever>. That's not a small change for everyone that uses us. It's pretty big.

But, because ESM is async, this is a change that has to be done. What I did was this: if you have tests using ESM in Node, you can still have your old sync code. But! If you have tests that are ESM, Mocha.run will be async and will return a promise.

I am fully aboard with breaking compatibility, and in the long run it had to be done, but I did want to give y'all the option of having it both ways. I can easily patch it to break compatibility and be "correct".

[giltayar]

In addition, if we expose loadFiles, which would have to be async if we break compatibility,
the code would be

mocha.addFile(...)
mocha.loadFiles().then(() => mocha.run()).then(() => console.log('Done')

[juergba]

@giltayar thank you.
If we exposed loadFiles, there would be no need to change Mocha#run. It could remain sync, always returning a Runner, right?

mocha.addFile(...)
mocha.loadFiles().then(() => mocha.run(() => console.log('Done')))

[juergba]

exports.singleRun = (mocha, {exit}, fileCollectParams) => {
  const files = collectFiles(fileCollectParams);
  debug('running tests with files', files);
  mocha.files = files;
  return mocha.run(exit ? exitMocha : exitMochaLater);
};

Couldn't we implement it here, even as semver-minor?

• experimental-module: true: we load our files asyncly outside of mocha.run(), mark the files somehow as loaded, then call mocha.run(). Files are already loaded, so we don't load them again inside of mocha.run().
• experimental-module: false: files are loaded synchly inside mocha.run(), as current state.

mocha.run would always return a Runner, no Promise.
watchRun: does not work with ems anyway (require.cache), so no changes here.

[giltayar]

@juergba yes, if we take out loadFiles, then run could be sync. But taking out loadFiles is anyway a breaking change.

Any in any case, singleRun couldn't be sync, as it has to have loadFiles in it. And that changes the interface anyway, right? So we still can't avoid it.

As for your idea of loadFiles being called outside, and we check whether it was already called or not, and if it was called, OK, and if not, load synchronously: it's still an awkward interface. Not clean and full of ifs and buts.

I say this: the correct solution is to make loadFiles and singleRun async, and that's a breaking change, whether or not loadFiles is inside run or not. So either go and break compatibility and be semver-major in a clean way, or we implement a weird solution, which could either be my solution, or yours. And I prefer my solution not because it's better (it's not!), but because people who are not native-node-ESM aware need not care about it at all. And, today, frankly, that's 99.9% of the people.

[juergba]

I prefer the clean way, semver-major yes/no can't really be a main criteria.
singleRun is a private function, turning it into async would not be a breaking change (?).

@demurgos @maxnordlund I need some help with this one and would highly appreciate your input.

[giltayar]

@juergba I will submit to the decision of my elders. 😀

Once you decide, I'll do the switch. It'll probably take no more than an hour or two.

Note that there are two decisions to make:

1. Do we enable run and others be async and thus breaking compat?
2. If we do, do we separate Mocha.prototype.loadFiles from Mocha.prototype.run and have only loadFiles be async?

[boneskull]

I know I'm probably blocking this, but I expect I can get to review it in about 10 days.

[David-Else]

THANK YOU @giltayar !! This is most definitely not just a 'nice to have' feature... can't believe that was added as a tag.

@boneskull Any update? Cheers!

[boneskull]

@giltayar IMO, zalgo is bad.

A third option would be to leave Mocha#run as-is and create a new method (e.g., runAsync) to implement the new behavior. We can call that function from the CLI, and we can continue to support Mocha#run until it's clear we don't want to support it any longer. I'd prefer not to break 3rd-party code relying on it (at least for the immediate future), even if we no longer use the API internally.

runAsync, then, would always return a Promise.

So... Promises. Because Mocha doesn't use any Promises internally (prior to this PR), we'll need to ensure Mocha's behavior on unhandled rejection is sane. Mocha has well-defined behavior around how it traps and handles uncaught exceptions. Rejection handling doesn't need to be the same as this, because it might not make sense, but if it's different, that behavior should be defined and documented.

If you're interested in this change, feel free to implement, but I can also run with this PR and get it over the finish line if you don't have time. Also what does @juergba think of this "new method" idea?

[boneskull]

Regarding "major" or "minor", I don't really care either way, but I still would rather not introduce a breaking API change lacking any clear necessity. If we can side-step the existing API for programmatic users, then IMO that's the best solution.

[juergba]

@giltayar using Mocha programmatically:

mocha.run(failures => process.exitCode = failures ? 1 : 0)
  .on('suite end', suite =>console.dir(suite));

There is no way to register event listeners on the runner instance when using Mocha@runAsync(), right?

[juergba]

@giltayar I did some testing on the copy-esm branch.

• I removed mocha.runAsync()
• call loadFilesAsync in singleRun and label files as loaded
• mocha.run: don't load a second time

mocha.addFile('../test.js');

mocha.loadFilesAsync()
  .then(() => mocha.run(failures => process.exitCode = failures ? 1 : 0)
  .on('suite end', function(suite) {
    console.dir(suite);
  })
);

mocha.loadFilesAsync would have to be public in this case.

[emma-borhanian]

I tried out this prerelease and ran into some problems/complexity.

Story

I'm interested in switching to mocha from jest because it supports native modules and I'm using an electron/React/sqlite stack for the productivity app I'm building. I'm already using type: "module" because I wanted a consistent import-style across my code and React convention is ESM.

Technical background

• I'm using babel (jsx, class properties, not for import-to-require) for React electron renderer-code and no transpilation for electron main code
• "scripts": { "mocha": "NODE_ENV=development node --experimental-specifier-resolution node --experimental-loader ./src/lib/mocha/transpiler.js -- node_modules/mocha/bin/_mocha --config ./config/mocha.config.cjs" }
• Using the still-unstable esm_transpiler_loader node API
  • to run babel on electron-renderer (React, jsx, etc) files
  • to skip electron css/asset imports

Problem

Node doesn't throw ERR_REQUIRE_ESM for .jsx files, instead throws SyntaxError: Cannot use import statement outside a module
https://github.com/mochajs/mocha/blob/v7.0.0-esm1/lib/esm-utils.js#L21
https://github.com/nodejs/node/blob/v13.8.0/lib/internal/modules/cjs/loader.js#L1161

Solution

// MAINTAIN(mocha-version): Hack: mocha wants an exception thrown on load to switch to ESM: https://github.com/mochajs/mocha/blob/v7.0.0-esm1/lib/esm-utils.js#L21
require.extensions['.jsx'] = function (module, filename) {
  // Emulate https://github.com/nodejs/node/blob/v13.8.0/lib/internal/modules/cjs/loader.js#L1167
  throw { code: 'ERR_REQUIRE_ESM' }
}

Aside: Many modules don't support named imports properly despite their documentation

Documentation suggests things like import React, {Component} from 'react' which are not compatible with type: "module" with the way those modules are written.

[emma-borhanian]

Node doesn't throw ERR_REQUIRE_ESM for .jsx files, instead throws SyntaxError: Cannot use import statement outside a module:
https://github.com/nodejs/node/blob/v13.8.0/lib/internal/modules/cjs/loader.js#L1161

I found a hacky workaround (link also provides context on how I ran into this problem).

Could you check package.json and import by default if the requiring package has set type: "module"? That wouldn't solve this problem for everyone but would for me, and seems correct regardless.

[juergba]

@emma-borhanian this issue is outside of this PR's scope, I would like to get it merged as is.
Afterwards the ESM implementation will grow and we are open to suggestions. But IMO checking for the nearest package.json (for each test file) is too expensive and actually Node's job.

[juergba]

@giltayar I rebased and resolved conflicts.

[juergba]

@giltayar I removed Mocha#runAsync and moved Mocha#loadFilesAsync up into singleRun.

Mocha#runAsync does not return Promise<Runner>, so we can't register events to the runner instance anymore.

yargs.handler (sync => async)
> runMocha (sync => async)

>> singleRun (sync => async)                      >> watchRun
>>> collectFiles                                  >>> collectFiles
>>> loadFilesAsync (new, async, public)

>>> mocha.run (remains sync, returns runner instance)
>>>> loadFiles (remains sync, evtl. skipped)
>>>> runner.run

[juergba]

@giltayar I pushed some changes due to code review and make-happy coveralls.

[giltayar]

@juergba - I saw what you did with removing Mocha#runAsync (by separating loading via Mocha#loadAsync and running via Mocha#run). I like it. Simpler.

All in all, I went over the changes you did, and they make sense. Thanks for the comprehensive review and changes!

[juergba]

@giltayar re Mocha#runAsync: I did this change because this method does not return Promise<Runner>. The possibility to register events to the runner is a must-have requirement when using Mocha programmatically.

Can I squash my commits into yours? You remain the owner this way, otherwise I will merge two separate commits.

Edit: do you agree with semver-minor?

[giltayar]

@juergba - you can squash, thanks. And, yes, semver-minor makes sense.

[juergba]

@giltayar thank you!
Lgtm, only PR's description could need some attention.