add zero-config support for loading ES modules in Node (round 2)

[jdalton]

This PR adds zero-config support for loading ES modules in Node. This PR just replaces the entry require with esmRequire. Now --require modules, configs, and others files can have ESM syntax. The caveat is that .mjs files are not supported since under the hood mocha is still using require() to load files. See the following esm doc note:

💡 By Node’s rules, builtin require cannot sideload .mjs files. However, with esm, ES modules can be sideloaded as .js files or .mjs files may be loaded with dynamic import.

@boneskull Feel free to push tests to this PR.

[coveralls]

Coverage increased (+0.08%) to 92.501% when pulling 58d4bc6 on jdalton:esm-master into 91b3a54 on mochajs:master.

[szb512]

Well, adding zero-config support will be beneficial for the tests, but then we would have to push lots of branches.

[juergba]

The parsing/loading of modules is a core functionality of Node. I'm unsure wether we should replace this by an external module.

I haven't understood the exact difference between using:

• Node's --experimental-modules flag
• mocha --require esm
• this PR

Are we considering this PR only to save the typing of --require esm ?
On the other hand we would need a new flag giving the user the possibility to opt out esm?

Anyway just changing "bin/_mocha" will not do the job. Since we don't spawn a child-process in all cases anymore, "bin/mocha" would need to be adapted as well.

[craigtaub]

@juergba good questions, will have a go at answering.

Node's --experimental-modules flag

i think it works with .js extension too.
e.g. (index.js is import assert from 'assert';)
node --experimental-modules index.js -> SyntaxError: Unexpected identifier. Works as .mjs
node -r esm index.js -> works

Also esm will work on node v6+ unlike the flag.

mocha --require esm

So AFAIK we sideload after the core code has run, so while this would work for spec files, it would not work for mochas core code. e.g. (an import inside cli.js)
node bin/mocha -r esm test/reporters/base.spec.js -> SyntaxError: Unexpected identifier
node -r esm bin/mocha test/reporters/base.spec.js -> works

this PR

It believe it acts like the working example above EXCEPT doesn't apply to the users spec file. So its the best of both. We can use ESM inside core without impacting users code.

Added to bin/mocha.

Let me know your thoughts on above. Its possible im completely out of my depth here lol.

[juergba]

It believe it acts like the working example above EXCEPT doesn't apply to the users spec file. So its the best of both. We can use ESM inside core without impacting users code.

Where should we allow ESM:

• loading our --require option?
• loading our --file option and user's test files?
• in Mocha's core code?
• parsing our configuration files?

IMO we should enable the first two items.
Enabling it for Mocha's core code, I don't see the benefit. There is a risk in overriding a Node core functionality with an external module, which is maintained by mainly just one maintainer.
I'm not convinced of enabling an esm zero-config support, with a flag --require esm it's ok though.

We should go for --experimental-modules. BTW the file extension can be handled with "type" field in package.json.

[juergba]

In my tests this didn't work. Overriding require with require = require('esm')(module); on L13 does work though.
Maybe there are more solutions.
Just loading the module esm as we do it in lib/cli/run-helpers.js is not working, additionally the require function has to be overridden, somewhere ...

[craigtaub]

In my tests this didn't work.

How were you testing? Seemed ok for me (for core code), been testing w/o any node flags tho.

[juergba]

I just tested this test file:

import {ok} from 'assert';
describe('fixture', function() {
  it('should be true', function() {
    ok(true);
  });
});

It does not run with the changes of this PR. It does run with --require esm.

[craigtaub]

Yeah I think esmRequire only gets applied to core code, so thats sounds right.

[craigtaub]

@juergba

Enabling it for Mocha's core code, I don't see the benefit.

Keeping up-to-date with language standards w/o a compilation-step feels a big benefit. Even once native ESM lands will be some time before we can drop support for un-supporting envs.

But its disappointing we have had so many issues (and concerns) with it.

[juergba]

Keeping up-to-date with language standards w/o a compilation-step feels a big benefit.

We are mainly still vegetating in ES5 caves.

I feel rather uncomfortable about this issue. It's not enough to take a few minutes time to say "it" runs and then merge this PR.
Someone has to take the lead and invest hours/days for this issue. What does "it" include? How are we going to test? What are we going to break (eg watch feature)?

#3006 is a good beginning to understand what our users' expectations are.

[juergba]

closed in lieu of #4038