Support --require of ESM; closes #4281 #4304

JacobLey · 2020-05-27T19:10:27Z

Allow files/modules specified in --require to be ESM.

CommonJS loading is still supported and the default.

Description of the Change

Support --requireing ESM files in config. Current behavior is to fail on ERR_REQUIRE_ESM.

Re-use existing "require or import" logic for loading test files to load required files.

Alternate Designs

Pipe requiring the ESM file through something like esm or ts-node. Although that seems much more complicated and overkill.

Why should this be in core?

Requiring JS files before the test suite is a core part of Mocha's functionality. This extends that support to ESM-first codebases, which is natively supported via "type":"module", or --exerimental-module.

Benefits

Allows developers more freedom in choosing to write ESM or CommonJS.

Possible Drawbacks

Native ESM in NodeJS is still labeled as experimental (although an --experimental flag is no longer needed). It is possible the usage will change, but that is justification for re-using existing "require or import" functionality.

Applicable issues

This should be an "enhancement" (minor release). --requireing CommonJS is still the default and fully supported, this will just allow additional support for ESM.

Allow files/modules specified in `--require` to be ESM. CommonJS loading is still supported and the default.

jsf-clabot · 2020-05-27T19:10:29Z

All committers have signed the CLA.

JacobLey · 2020-05-27T19:12:10Z

No tests included in this first pass.

I don't see any existing tests for this... so not exactly sure where to put them.

JacobLey · 2020-05-27T19:13:28Z

lib/esm-utils.js

@@ -15,7 +12,7 @@ const requireOrImport = async file => {
    return require(file);
  } catch (err) {
    if (err.code === 'ERR_REQUIRE_ESM') {
-      return import(url.pathToFileURL(file));


Not sure what url.pathToFileURL was achieving?
import() allows a plain string, and this way the required file can still be resolvable via node's logic (e.g. loading from node_modules)

I guess it was required for Windows.
Gosh I hate windows

JacobLey · 2020-05-27T19:14:19Z

lib/esm-utils.js

 const path = require('path');

-const requireOrImport = async file => {
-  file = path.resolve(file);


Moved resolve() to loadFilesAsync so the file doesn't have to be a fully-resolved name, but can be a package from node_modules as well.

coveralls · 2020-05-27T19:35:20Z

Coverage increased (+0.01%) to 93.445% when pulling c411c30 on JacobLey:requireESM into 6d60eb0 on mochajs:master.

Windows compatible

boneskull

Thanks! Great work. Can you please add an integration test in test/integration/options/require.spec.js which loads an ESM module fixture via --require?

boneskull · 2020-05-28T17:23:51Z

apologies, the organization of test is often not obvious. probably need to add more info to test/README.md

As both .mjs and type=module (combined with cjs for good measure). Updated linter to allow tests to use spread operator (ecmaVersion 2018) Allow --require'd module to be an object, or "module"

JacobLey · 2020-05-28T20:24:26Z

test/integration/options/require.spec.js

+            ],
+            {
+              env: {
+                ...process.env,


This is what I updated the parser options (parserOptions: 2018) in eslintrc for.

Considering mocha is documented with:

As of v7.0.0, Mocha requires Node.js v8.0.0 or newer.

This seems safe enough for tests at least.

If there is an easier way to pass --experimental-modules to the mocha (instead of calling node directly) happy to use that instead...

I apologize for the state of things (we're working on it; see #4293), but...

I wanted to get a subset of the integration tests (test/integration/) running in a browser, but haven't completed this work. Stuff in test/node-unit/ (and a few other places) is allowed to use ES2018 syntax, but basically everything else under test/ is limited to ES5. It's probably OK to allow ES2018 in test/integration/, if we expect #4293 to get merged in the near future, since we'll be able to transpile everything down to where it needs to be. I don't envision browser-based integration tests landing before then, so IMO it's fine. Others may disagree (@mochajs/core ?)

However, stuff in test/unit/ and test/browser cannot, at this time, be ES2018, because these tests run in IE11. So, please revert the change to .eslintrc.yml, then add 'test/integration/**/*.js' to the list of overrides which already use parserOptions: 2018 (L39 or so).

Here, you're running bin/mocha (which is what invokeMocha does), but using the NODE_OPTIONS environment variable to set --experimental-modules. You don't need to do this; just pass --experimental-modules in your array of args. bin/mocha knows what to do with it.

e.g.

invokeMocha(['path/to/fixture', '--some-other-flag'].concat(+process.versions.node.split('.')[0] >= 13 ? [] : ['--experimental-modules'], function() {})

I realize this means you can just revert the change to .eslintrc.yml entirely, which is maybe what should happen.

Reverted change to eslintrc

Didn't realize I could pass the flag directly though mocha! Since I don't need any lint changes, I don't think this is the place to reorganize what is allowed where.

JacobLey · 2020-05-28T20:37:47Z

lib/cli/run-helpers.js

@@ -92,7 +92,10 @@ exports.handleRequires = async (requires = []) => {
      debug('resolved required file %s to %s', mod, modpath);
    }
    const requiredModule = await requireOrImport(modpath);
-    if (type(requiredModule) === 'object' && requiredModule.mochaHooks) {
+    if (
+      ['object', 'module'].includes(type(requiredModule)) &&


node -e "import('./test/integration/fixtures/options/require/root-hook-defs-esm.fixture.mjs').then(mod => console.log(Object.prototype.toString.call(mod)))"
-> [object Module]

Piped through the type function: https://github.com/mochajs/mocha/blob/master/lib/utils.js#L216
Returns module.

So added handling for that "module" case (which seems reasonable considering that is what we would expect from ESM... but for the sake of discussing alternatives, could add some handling to type() to map module -> object

no, this seems correct. but, we should also add a test in test/node-unit/utils.spec.js to cover the behavior. you don't need to do this unless you want to.

we can change it to return object if there are unforeseen consequences. what does typeof requiredModule return?

node -e "import('./test/integration/fixtures/options/require/root-hook-defs-esm.fixture.mjs').then(mod => console.log(typeof mod))" -> object

I'll let you interpret that... since there must be some reason not to use typeof directly.

add a test to cover the behavior

I can add that as well, so long as we are fine with Modules returning "module"

there's no real great reason to use type() here other than trying to maintain some consistency. we should use typeof and not worry about it.

boneskull

pls see comments

boneskull · 2020-05-28T20:33:50Z

test/integration/fixtures/options/require/esm/root-hook-defs-esm.fixture.js

@@ -0,0 +1,8 @@
+export const mochaHooks = () => ({


boneskull · 2020-05-28T20:37:59Z

test/integration/options/require.spec.js

+            ],
+            {
+              env: {
+                ...process.env,


Here, you're running bin/mocha (which is what invokeMocha does), but using the NODE_OPTIONS environment variable to set --experimental-modules. You don't need to do this; just pass --experimental-modules in your array of args. bin/mocha knows what to do with it.

e.g.

invokeMocha(['path/to/fixture', '--some-other-flag'].concat(+process.versions.node.split('.')[0] >= 13 ? [] : ['--experimental-modules'], function() {})

I realize this means you can just revert the change to .eslintrc.yml entirely, which is maybe what should happen.

Add truthy check to handle null edge case type(ES Module) => "module", but we treat it the same as an object

boneskull

LGTM, thanks. Would like @giltayar to take a gander, if possible

giltayar

LGTM.

giltayar · 2020-05-29T03:34:45Z

lib/esm-utils.js

@@ -25,7 +30,7 @@ const requireOrImport = async file => {
 exports.loadFilesAsync = async (files, preLoadFunc, postLoadFunc) => {
  for (const file of files) {
    preLoadFunc(file);
-    const result = await requireOrImport(file);
+    const result = await exports.requireOrImport(path.resolve(file));


If the file in question is a package name, path.resolve will fail (AFAIK with an exception). Why did you add a path.resolve here?

Oh, I see. --require-d files can be module names, and so you didn't want requireOrImport, which is now used also in handleRequire, to resolve them to a path. So you moved it to loadFileAsync, where it is needed (for Windows, it seems...)

juergba

@JacobLey please also adapt the esm limitation section in our docs. Thank you.

giltayar · 2020-05-29T17:59:05Z

Because "require" is not typically a word used with ESM ("import" is more generally used), I would add specific messaging in the --require documentation that says that it also accepts ES modules.

juergba

I just have two questions about resolving the relative path / absolute path / package name (bare specifier). I'm unsure about this.

The docs states:

Bare specifiers, and the bare specifier portion of deep import specifiers, are strings; but everything else in a specifier is a URL.

So the relative path should be an URL as well?

juergba · 2020-06-01T09:08:00Z

test/integration/options/require.spec.js

+          invokeMochaAsync(
+            [
+              '--require=' +
+                require.resolve(


--requireworks with absolute path and module names. Does it work also with relative paths?

Yes, see where you commented on handleRequires, relative paths are transformed into absolute paths.

None of the tests use it though because the actual CWD isn't very clear in-test. But maybe it is possible to calculate a relative path in test and pass that through as well.

Again, that particular functionality is unchanged in this PR

juergba · 2020-06-01T09:17:24Z

lib/cli/run-helpers.js

-  requires.reduce((acc, mod) => {
+exports.handleRequires = async (requires = []) => {
+  const acc = [];
+  for (const mod of requires) {
    let modpath = mod;
    // this is relative to cwd
    if (fs.existsSync(mod) || fs.existsSync(`${mod}.js`)) {
      modpath = path.resolve(mod);


If the file in question is a package name, path.resolve will fail [...]

Will this work for packages?

It should, fs.existsSync will return false for a package name, so it is not resolved at all (passed raw) to requireOrImport (e.g. requireOrImport('@babel/register')

This behavior is unchanged in this PR

boneskull · 2020-06-01T22:50:39Z

docs/index.md

@@ -1007,6 +1007,8 @@ Modules required in this manner are expected to do work synchronously; Mocha won

 Note you cannot use `--require` to set a global `beforeEach()` hook, for example &mdash; use `--file` instead, which allows you to specify an explicit order in which test files are loaded.

+> As of v7.3.0, Mocha supports `--require` for [NodeJS native ESM](#nodejs-native-esm-support). There is no separate `--import` flag.


note that the version here may need changing

boneskull · 2020-06-01T22:53:05Z

LGTM. Thanks much @JacobLey !

Support --require of ESM; closes mochajs#4281

5ef04be

Allow files/modules specified in `--require` to be ESM. CommonJS loading is still supported and the default.

JacobLey commented May 27, 2020

View reviewed changes

Conditionally generate url for import

f28fd67

Windows compatible

boneskull suggested changes May 28, 2020

View reviewed changes

boneskull added type: feature enhancement proposal semver-minor implementation requires increase of "minor" version number; "features" labels May 28, 2020

Add tests for --require ESM

e9834fd

As both .mjs and type=module (combined with cjs for good measure). Updated linter to allow tests to use spread operator (ecmaVersion 2018) Allow --require'd module to be an object, or "module"

JacobLey force-pushed the requireESM branch from c9d1930 to e9834fd Compare May 28, 2020 20:21

JacobLey commented May 28, 2020

View reviewed changes

JacobLey requested a review from boneskull May 28, 2020 20:38

boneskull suggested changes May 28, 2020

View reviewed changes

JacobLey added 2 commits May 28, 2020 16:41

Revert change to eslintrc, use mocha to pass experimental flag

ebc2a07

Replace type() -> typeof

c411c30

Add truthy check to handle null edge case type(ES Module) => "module", but we treat it the same as an object

boneskull requested a review from giltayar May 28, 2020 21:59

boneskull approved these changes May 28, 2020

View reviewed changes

giltayar approved these changes May 29, 2020

View reviewed changes

juergba reviewed May 29, 2020

View reviewed changes

Remove doc limitation for --require ESM

a6029b9

JacobLey requested a review from juergba May 29, 2020 14:43

Add note to --require docs about ESM support

cc401ce

juergba reviewed Jun 1, 2020

View reviewed changes

boneskull reviewed Jun 1, 2020

View reviewed changes

boneskull merged commit 273dbbb into mochajs:master Jun 1, 2020

boneskull added this to the next milestone Jun 1, 2020

boneskull mentioned this pull request Jun 3, 2020

update docs to show that ES modules can be used with root hook plugins #4310

Closed

boneskull modified the milestones: next, v8.0.0 Jun 10, 2020

lieser mentioned this pull request Jul 25, 2020

Support require of ESM files hbenl/vscode-mocha-test-adapter#122

Closed

Omrisnyk mentioned this pull request Dec 7, 2023

[Snyk] Fix for 1 vulnerabilities Omrisnyk/npm-lockfiles#131

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support --require of ESM; closes #4281 #4304

Support --require of ESM; closes #4281 #4304

JacobLey commented May 27, 2020 •

edited

Loading

jsf-clabot commented May 27, 2020 •

edited

Loading

JacobLey commented May 27, 2020

JacobLey May 27, 2020

JacobLey May 28, 2020

JacobLey May 27, 2020

coveralls commented May 27, 2020 •

edited

Loading

boneskull left a comment

boneskull commented May 28, 2020

JacobLey May 28, 2020 •

edited

Loading

boneskull May 28, 2020 •

edited

Loading

boneskull May 28, 2020

JacobLey May 28, 2020

JacobLey May 28, 2020

boneskull May 28, 2020

boneskull May 28, 2020

JacobLey May 28, 2020 •

edited

Loading

boneskull May 28, 2020

boneskull May 28, 2020

boneskull left a comment

boneskull May 28, 2020

boneskull May 28, 2020

boneskull left a comment

giltayar left a comment

giltayar May 29, 2020

giltayar May 29, 2020

juergba left a comment

giltayar commented May 29, 2020

juergba left a comment

juergba Jun 1, 2020

JacobLey Jun 1, 2020

juergba Jun 1, 2020

JacobLey Jun 1, 2020

boneskull Jun 1, 2020

boneskull commented Jun 1, 2020

		@@ -1007,6 +1007,8 @@ Modules required in this manner are expected to do work synchronously; Mocha won

		Note you cannot use `--require` to set a global `beforeEach()` hook, for example — use `--file` instead, which allows you to specify an explicit order in which test files are loaded.

		> As of v7.3.0, Mocha supports `--require` for [NodeJS native ESM](#nodejs-native-esm-support). There is no separate `--import` flag.

Support --require of ESM; closes #4281 #4304

Support --require of ESM; closes #4281 #4304

Conversation

JacobLey commented May 27, 2020 • edited Loading

Description of the Change

Alternate Designs

Why should this be in core?

Benefits

Possible Drawbacks

Applicable issues

jsf-clabot commented May 27, 2020 • edited Loading

JacobLey commented May 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 27, 2020 • edited Loading

boneskull left a comment

Choose a reason for hiding this comment

boneskull commented May 28, 2020

JacobLey May 28, 2020 • edited Loading

Choose a reason for hiding this comment

boneskull May 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobLey May 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull left a comment

Choose a reason for hiding this comment

giltayar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juergba left a comment

Choose a reason for hiding this comment

giltayar commented May 29, 2020

juergba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull commented Jun 1, 2020

JacobLey commented May 27, 2020 •

edited

Loading

jsf-clabot commented May 27, 2020 •

edited

Loading

coveralls commented May 27, 2020 •

edited

Loading

JacobLey May 28, 2020 •

edited

Loading

boneskull May 28, 2020 •

edited

Loading

JacobLey May 28, 2020 •

edited

Loading