🐛 Bug: .mjs test files can be loaded with `require()` which breaks custom module loaders

[trentm]

Bug Report Checklist

• I have read and agree to Mocha's Code of Conduct and Contributing Guidelines
• I have searched for related issues and issues with the faq label, but none matched my issue.
• I have 'smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, my usage of Mocha, or Mocha itself.
• I want to provide a PR to resolve this

Expected

The recent changes in mocha@11.7.x for deciding whether to load a mocha test file as CommonJS or ESM should not use CommonJS for .mjs files. Specifically I mean for the code path that runs requireModule() in lib/nodejs/esm-utils.js.

Actual

"esm-utils.js" includes:

if (process.features.require_module) {
  exports.requireOrImport = requireModule;

which leads to require(file) being used to load a test file if running with a version of node that supports the require(esm) feature. However:

1. Versions of Node.js v20 starting with v20.19.0 include require(esm) support (and hence pass the if (process.features.require_module) condition), but do not include implement module.registerHooks() to run synchronous module customization hooks in thread nodejs/node#55698 (and possibly other Node.js module hook handling changes, I'm not perfectly clear on which Node.js commits are relevant). The result is that a custom module loader is not used for that loaded file, or the files it later imports.
2. All versions of Node.js that support require(esm) and have the additional module hook handling changes (e.g. Node.js ~22.15.0, some minor version of 24.x), will run custom module loader hooks for the imports in a file loaded by require(some-esm-file), but will NOT run the custom module loader hooks for the some-esm-file itself.

tl;dr: Even in the latest versions of Node.js, doing require("./some-esm-file.mjs") has different behaviour with custom module loaders compared to doing await import("./some-esm-file.mjs")

Minimal, Reproducible Example

https://github.com/trentm/repro-mocha-11-mjs-import-hooks-issue

This is a small example with a test file that (a) uses .mjs to indicate to Node that it should be loaded as an ES module, and (b) depends on a loader hook (--experimental-loader=./hook.mjs) to be run when the test file is loaded for the tests to pass.

NODE_NO_WARNINGS=1 node --experimental-loader=./hook.mjs ./node_modules/.bin/mocha 'test/**/*.test.mjs'

The README shows that this test passes when using mocha@11.6.0, but fails with mocha@11.7.x. You must run with Node.js v20.19.x or v22.14.0 to see both test cases fail. Running with v22.15.0 or later will fail one of the two test cases.

Versions

% npx mocha --version
11.7.1
% node --version
v20.19.1

Additional Info

A believe a good fix would be to make this change:

diff --git a/lib/nodejs/esm-utils.js b/lib/nodejs/esm-utils.js
index 3cea8af2..773f0829 100644
--- a/lib/nodejs/esm-utils.js
+++ b/lib/nodejs/esm-utils.js
@@ -90,6 +90,10 @@ const tryImportAndRequire = async (file, esmDecorator) => {
 // and `require.cache` effective, while allowing us to load ESM modules
 // and CJS modules in the same way.
 const requireModule = async (file, esmDecorator) => {
+  if (path.extname(file) === '.mjs') {
+    return formattedImport(file, esmDecorator);
+  }
+
   try {
     return require(file);
   } catch (err) {

This does the same ".mjs" guard as was already added for the tryImportAndRequire function in the same file. The idea is that the ".mjs" extension is a clear sign that the file should be loaded with import() and not with require().

[trentm]

The concrete example where we hit this bug is here: open-telemetry/opentelemetry-js#5850 (comment)

[mark-wiemer]

Excellent bug, description, and proposed fix, thank you! I will look into this and open a PR with affected Node versions isolated for clarity soon :)

[mark-wiemer]

I'm curious for your thoughts around including a similar guard for cjs files? I haven't look at the Node code... ever... :D

[mark-wiemer]

Hey @trentm, I'm having some trouble repro'ing, I keep getting issues like this in Windows (Bash for Windows) and WSL (Ubuntu):

/mnt/c/Users/markw/my-stuff/mocha-stuff/mocha/sandbox/issue-5425 (main)
$ NODE_NO_WARNINGS=1 node --experimental-loader=./hook.mjs ./node_modules/.bin/mocha 'test/**/*.test.mjs'
/mnt/c/Users/markw/my-stuff/mocha-stuff/mocha/sandbox/issue-5425/node_modules/.bin/mocha:2
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")
          ^^^^^^^

SyntaxError: missing ) after argument list
    at wrapSafe (node:internal/modules/cjs/loader:1472:18)
    at Module._compile (node:internal/modules/cjs/loader:1501:20)
    at Module._extensions..js (node:internal/modules/cjs/loader:1613:10)
    at Module.load (node:internal/modules/cjs/loader:1275:32)
    at Module._load (node:internal/modules/cjs/loader:1096:12)
    at cjsLoader (node:internal/modules/esm/translators:298:15)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:240:7)
    at ModuleJob.run (node:internal/modules/esm/module_job:263:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:540:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:117:5)

Node.js v20.19.2

This is similar whether I use mocha, _mocha, _mocha.cmd, or _mocha.ps1 :( I'll switch to my Linux Mint partition, but if you know of a Windows-compatible or cross-platform repro command that'd be helpful

Edit: The issue does repro as you described on Linux Mint Cinnamon, so I should be able to get something started here :)

[trentm]

I'm curious for your thoughts around including a similar guard for cjs files?

Per the rules described at https://nodejs.org/api/all.html#all_cli_program-entry-point
I think it would be fine to attempt to have the mocha file loading code restrict to using require(...) for .cjs files.

Doing so, would I think look something like this, possibly also with something similar at the top of requireModule in the same file:

diff --git a/lib/nodejs/esm-utils.js b/lib/nodejs/esm-utils.js
index 3cea8af2..cec6cbb7 100644
--- a/lib/nodejs/esm-utils.js
+++ b/lib/nodejs/esm-utils.js
@@ -39,8 +39,11 @@ exports.doImport = async file => import(file);
 // TypeScript support is enabled, `.ts` files are also loaded using `import()`, and
 // compilers based on `require.extensions` are omitted.
 const tryImportAndRequire = async (file, esmDecorator) => {
-  if (path.extname(file) === '.mjs') {
+  const ext = path.extname(file);
+  if (ext === '.mjs') {
     return formattedImport(file, esmDecorator);
+  } else if (ext === '.cjs') {
+    return require(file);
   }
   try {
     return dealWithExports(await formattedImport(file, esmDecorator));

I haven't tested this at all.

I will look into this and open a PR

Thanks!

I keep getting issues like this in Windows (Bash for Windows) and WSL (Ubuntu):

Sorry, my bad.
I think this should be fixed by: trentm/repro-mocha-11-mjs-import-hooks-issue@507301e
I don't have easy access to a Windows machine to test it, but pretty sure that is correct.

[mark-wiemer]

Thanks for the docs :) I'll keep this one open until I'm more confident on whether we should explicitly check for cjs. The docs say that Node does check, and that matches my experience, so it feels like we should too. But I don't want to mess with this code any more than we have to :/

I don't want someone to open a bug ".cjs test files can be loaded with import()", but I don't have the expertise yet to reproduce that. For now, I think, I'll be kicking this can down the road, as a lot of these features are still experimental and I want to learn a lot before changing any code here.

btw, this repro convinced me to start using my Linux partition as my main machine, if I switch back to Windows I'll let you know if that new command works :D (it looks like it does, fwiw)

PR should be merged early next week :)