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

Node esm support #4038

Open
wants to merge 3 commits into
base: master
from
Open

Node esm support #4038

wants to merge 3 commits into from

Conversation

@giltayar
Copy link

giltayar commented Oct 1, 2019

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

This comment has been minimized.

Copy link

jsf-clabot commented Oct 1, 2019

CLA assistant check
All committers have signed the CLA.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 1, 2019

Coverage Status

Coverage decreased (-0.09%) to 92.724% when pulling 844991d on giltayar:node-esm-support into 69339a3 on mochajs:master.

@giltayar

This comment has been minimized.

Copy link
Author

giltayar commented Oct 1, 2019

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 giltayar force-pushed the giltayar:node-esm-support branch from 44fa39c to f9f92f8 Oct 1, 2019
@giltayar

This comment has been minimized.

Copy link
Author

giltayar commented Oct 1, 2019

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 comment has been minimized.

Copy link
Member

juergba commented Oct 3, 2019

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

This comment has been minimized.

Copy link
Author

giltayar commented Oct 4, 2019

@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

This comment has been minimized.

Copy link
Author

giltayar commented Oct 4, 2019

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

This comment has been minimized.

Copy link
Member

juergba commented Oct 4, 2019

@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

This comment has been minimized.

Copy link
Member

juergba commented Oct 4, 2019

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

This comment has been minimized.

Copy link
Author

giltayar commented Oct 4, 2019

@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

This comment has been minimized.

Copy link
Member

juergba commented Oct 6, 2019

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

This comment has been minimized.

Copy link
Author

giltayar commented Oct 6, 2019

@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

This comment has been minimized.

Copy link
Member

boneskull commented Oct 12, 2019

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

@David-Else

This comment has been minimized.

Copy link

David-Else commented Nov 22, 2019

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 boneskull removed the nice-to-have label Nov 22, 2019
@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Nov 22, 2019

@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

This comment has been minimized.

Copy link
Member

boneskull commented Nov 22, 2019

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

This comment has been minimized.

Copy link
Member

juergba commented Jan 5, 2020

@giltayar this PR hasn't got any more comments by maintainers. So I have taken some decisions and propose:

  • I would like to publish this PR with an experimental release as eg. v7.0.0-esm1 or whatever. This can be done within days, probably.
  • please rebase with master
  • I prefer not to include mocharc at this time, please remove. It has no priority and I would like to grow slowly into esm. I'm not going to review this part, but keep it for later.
  • we have errors to fix (esm package, watch mode) and warnings to avoid.

I hope you are still on board. Thank you.

@giltayar

This comment has been minimized.

Copy link
Author

giltayar commented Jan 6, 2020

@juergba Definitely still on board, and will rebase tonight! I will also revert the mocharc code. It's definitely a lot of code changes (from sync to async), so maybe a separate PR is a good idea (will also do that once ESM is in).

@giltayar giltayar force-pushed the giltayar:node-esm-support branch 2 times, most recently from 6a46f26 to 0b9f14c Jan 6, 2020
@giltayar

This comment has been minimized.

Copy link
Author

giltayar commented Jan 6, 2020

@juergba - rebased (without the mocharc change) and passed tests.

@juergba

This comment has been minimized.

Copy link
Member

juergba commented Jan 7, 2020

@giltayar thank you! I will review until tomorrow.

Edit: your branch is not rebased correctly with master, the latest commits are missing.

Copy link
Member

juergba left a comment

[...] you're right, it's because of the way we import/require. You can silence it by adding --no-warning to Mocha.
But it's definitely a problem that we'll have to solve, either by figuring out the type of the file, or by always import-ing. Something to think about...

What is the state of this warning problem?

lib/cli/cli.js Outdated Show resolved Hide resolved
lib/cli/cli.js Outdated Show resolved Hide resolved
@@ -87,7 +87,7 @@ exports.builder = yargs =>
},
extension: {
default: defaults.extension,
defaultDescription: 'js',
defaultDescription: 'js,cjs,mjs',

This comment has been minimized.

Copy link
@juergba

juergba Jan 8, 2020

Member

Remove this line, it's not necessary (see watch-ignore for example).

This comment has been minimized.

Copy link
@giltayar

giltayar Jan 10, 2020

Author

Why is it not necessary? I looked at watch-ignore but couldn't understand why this was so. Don't we want to say that the default extensions we look at are those?

This comment has been minimized.

Copy link
@juergba

juergba Jan 10, 2020

Member

default property does this job already, defaultDescription just overwrites it. If you have a look at the --help output, --watch-ignore shows everything as needed.

} catch (err) {
console.error(err.stack || `Error: ${err.message || err}`);
process.exit(1);
}

This comment has been minimized.

Copy link
@juergba

juergba Jan 8, 2020

Member

I'm unsure wether this will be sufficient to avoid "unhandled promise rejections". When a promise further down the call stack is failing and not catched. We will see ....

This comment has been minimized.

Copy link
@giltayar

giltayar Jan 10, 2020

Author

If we're dealing with all the promises down the call stack correctly, then there shouldn't be a problem.

package.json Outdated Show resolved Hide resolved
@juergba

This comment has been minimized.

Copy link
Member

juergba commented Jan 10, 2020

To suppress the warning, you could temporarily override the function process.emitWarning before loading the files, and set it back to its original value after loading.

@sebamarynissen

This comment has been minimized.

Copy link

sebamarynissen commented Jan 10, 2020

@juergba Won't that be a problem if you require a test file that somehow calls process.emitWarning? By overriding it this would cause the warning to not be emitted. Unless of course you only filter out somehow the (node:21944) Warning: require() of ES modules is not supported. and let the other warnings come through.

@juergba

This comment has been minimized.

Copy link
Member

juergba commented Jan 10, 2020

IMO no, if we override just for loading the files, eg in loadFilesAsync(). Running the test files - which possibly emit warnings - comes afterwards.
The order is: loading the test files, then parsing the test runner tree, then running the tests.

@sebamarynissen

This comment has been minimized.

Copy link

sebamarynissen commented Jan 10, 2020

That's true, but I think it's still possible if you have something like this

// my-test.js
require('./test-setup.js');
describe('...', function() {});

// test-setup.js
process.emitWarning('Some important warning');

Personally, I prefer just using import() for everything - as @boneskull proposed - rather than overriding process.emitWarning which feels quite hacky.

@juergba

This comment has been minimized.

Copy link
Member

juergba commented Jan 10, 2020

The goal of this PR is to be merged into master after having proven stable. We can't publish code with experimental state in our productive release. import() still has experimental stability.

Edit: publishing yes, but not using import() for everything.

@giltayar

This comment has been minimized.

Copy link
Author

giltayar commented Jan 10, 2020

@juergba @sebamarynissen - I'll try and override process.emitWarning locally (only for that specific warning) during the require.

@giltayar

This comment has been minimized.

Copy link
Author

giltayar commented Jan 10, 2020

@sebamarynissen are you sure this still happens. I just used Node.js v13.6.0, with a test.js file inside a folder with a package.json with type: module. The only warning I got was (node:39080) ExperimentalWarning: The ESM module loader is experimental.

Could it be that they removed the warning you saw? Or maybe I didn't understand the scenario?

@giltayar giltayar force-pushed the giltayar:node-esm-support branch from dd8c113 to 844991d Jan 10, 2020
@giltayar

This comment has been minimized.

Copy link
Author

giltayar commented Jan 10, 2020

@juergba - rebased again. It is now rebased against the latest commit.

@sebamarynissen

This comment has been minimized.

Copy link

sebamarynissen commented Jan 10, 2020

@sebamarynissen are you sure this still happens. I just used Node.js v13.6.0, with a test.js file inside a folder with a package.json with type: module. The only warning I got was (node:39080) ExperimentalWarning: The ESM module loader is experimental.

Could it be that they removed the warning you saw? Or maybe I didn't understand the scenario?

@giltayar I'm using node 13.3 (on Windows 10, but shouldn't matter) and I still get there error:

(node:3452) Warning: require() of ES modules is not supported.
require() of C:\test.js from C:\...\npm\node_modules\mocha\lib\esm-utils.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename test.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from C:\package.json.

My test setup:

  • a package.json that only contains {"type": "module"}
  • a an empty test file with .js extension: test.js.

If I run mocha test.js I get the error above.

@giltayar

This comment has been minimized.

Copy link
Author

giltayar commented Jan 10, 2020

@sebamarynissen - I just tried it using 13.3 (using NVM) and got your warning. Then retried with 1.3.6, and didn't get the warning! Tried 13.4, and didn't get the warning.

So it seems that this warning was removed from v13.4.

@juergba - if it's OK with you, I believe we shouldn't do anything to deal with this problem, given that it happens only if you use ESM (with type: module), and only when you use some specific versions of Node.js.

@juergba

This comment has been minimized.

Copy link
Member

juergba commented Jan 10, 2020

@giltayar Ok, we leave the warning problem as is.

Could you have a look at the changelog in branch release/v7.0.0-esm1, please?

@giltayar

This comment has been minimized.

Copy link
Author

giltayar commented Jan 11, 2020

@juergba - changelog LGTM.

@juergba

This comment has been minimized.

Copy link
Member

juergba commented Jan 13, 2020

We published yesterday an experimental release: npm i mocha@7.0.0-esm1
Release notes

@giltayar thank you!

@maxwell-k

This comment has been minimized.

Copy link

maxwell-k commented Jan 13, 2020

I have tried the experimental release using Node v12.14.0 and v13.1.0 and it works as expected, thank you!

@DominusVilicus

This comment has been minimized.

Copy link

DominusVilicus commented Jan 16, 2020

Had to reinstall mocha globally with npm i -g mocha@7.0.0-esm1 I think it was causing some errors for me until I did that, and 13.3 wasn't working either for some reason, not sure if it was because of the global installation issue

@giltayar

This comment has been minimized.

Copy link
Author

giltayar commented Jan 16, 2020

@DominusVilicus - 13.3 was a problematic version (see above comments for a reference to a problem we found). Could you try 13.4 and above, without the global installation? It should work also without it being globally installed. If the problem persists, could you perhaps elaborate on what it was?

@sebamarynissen

This comment has been minimized.

Copy link

sebamarynissen commented Jan 16, 2020

I had a similar issue when updating my global mocha from your node-esm-support branch to 7.0.0-esm1. I uninstalled mocha globally, then reinstalled and everything was fine.

I can still reproduce the issue though when I install mocha again using npm i -g mocha@7.0.0-esm1. The error I get:

internal/modules/cjs/loader.js:964
    throw err;
    ^

Error: Cannot find module 'glob'
Require stack:
- C:\...\AppData\Roaming\npm\node_modules\mocha\lib\utils.js
- C:\...\AppData\Roaming\npm\node_modules\mocha\bin\mocha
�[90m    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:961:17)�[39m
�[90m    at Function.Module._load (internal/modules/cjs/loader.js:854:27)�[39m
�[90m    at Module.require (internal/modules/cjs/loader.js:1023:19)�[39m
�[90m    at require (internal/modules/cjs/helpers.js:72:18)�[39m
    at Object.<anonymous> (C:\Users\sebas\AppData\Roaming\npm\node_modules\�[4mmocha�[24m\lib\utils.js:15:12)
�[90m    at Module._compile (internal/modules/cjs/loader.js:1128:30)�[39m
�[90m    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1167:10)�[39m
�[90m    at Module.load (internal/modules/cjs/loader.js:983:32)�[39m
�[90m    at Function.Module._load (internal/modules/cjs/loader.js:891:14)�[39m
�[90m    at Module.require (internal/modules/cjs/loader.js:1023:19)�[39m {
  code: �[32m'MODULE_NOT_FOUND'�[39m,

Is there perhaps something wrong with the dependencies?

@giltayar

This comment has been minimized.

Copy link
Author

giltayar commented Jan 16, 2020

@sebamarynissen I just tried it (npm i -g mocha@7.0.0-esm1 in Node v13.6, and ran a small test with a passing and failing test), and it worked well for me.

I also checked the dependencies, both in the code and in the raw package in the npm registry, and glob is definitely there.

@sebamarynissen

This comment has been minimized.

Copy link

sebamarynissen commented Jan 16, 2020

@giltayar Yeah I did some more tests and now I can't reproduce it anymore. Seems like npm messed up the installation somehow because not only glob was not there, there was only one module in mocha/node_modules - guess it was es-abstract, but I don't think that's relevant.

I guess that this is effectively the issue @DominusVilicus was facing as well. Perhaps related to node 13.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.