Missing file/package in import statement misreported as ESM error

[JakobJingleheimer]

Prerequisites

• Checked that your issue hasn't already been filed by cross-referencing issues with the faq label
• Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
• '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, your usage of Mocha, or Mocha itself
• Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with: node node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend that you not install Mocha globally.

Description

This lead me down the garden path: A bad import statement (where the targeted package doesn't exist) results in mocha claiming the containing file was not interpreted as ESM despite containing an import (and mocha swallows the actual, correct error message).

Error from Mocha:

/…/test.jsx:1
import {
^^^^^^

SyntaxError: Cannot use import statement outside a module

Error from Node.js:

Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@testing-library/jest-dom'
imported from /…/test.jsx

The bug originates from https://github.com/mochajs/mocha/blob/v9.1.3/lib/nodejs/esm-utils.js#L51

Steps to Reproduce

test.jsx

Without installing @testing-library/jest-dom:

import '@testing-library/jest-dom';

NODE_ENV=test NODE_OPTIONS="--loader=./loader.mjs" npx mocha ./test.jsx

(The loader live-transpiles jsx via esbuild and marks the file as esm; it's not especially important for the reproduction but it did help me find the bug in Mocha).

Expected behavior: Mocha reports the correct error (that the targeted package doesn't exist, as node correctly reports).

Actual behavior: Mocha wrongly reports a different, unrelated error.

Reproduces how often: 100%

Versions

• The output of mocha --version and node node_modules/.bin/mocha --version: 9.1.3 (also occurs in 8.3.0)
• The output of node --version: 17.2.0
• Your operating system
  • name and version: macOS 12.1
  • architecture (32 or 64-bit): 64 bit
• Your shell (e.g., bash, zsh, PowerShell, cmd): bash
• Your browser and version (if running browser tests): N/A
• Any third-party Mocha-related modules (and their versions):
• Any code transpiler (e.g., TypeScript, CoffeeScript, Babel) being used (and its version): esbuild

Additional Information

N/A

[JakobJingleheimer]

I believe the checks for ERR_MODULE_NOT_FOUND and ERR_UNKNOWN_FILE_EXTENSION as they are here are both wrong/outdated:

ERR_MODULE_NOT_FOUND: I see the code comment about the red herring:

mocha/lib/nodejs/esm-utils.js

Lines 58 to 65 in 28b4824

	if (requireErr.code === 'ERR_REQUIRE_ESM') {
	// This happens when the test file is a JS file, but via type:module is actually ESM,
	// AND has an import to a file that doesn't exist.
	// This throws an `ERR_MODULE_NOT_FOUND` // error above,
	// and when we try to `require` it here, it throws an `ERR_REQUIRE_ESM`.
	// What we want to do is throw the original error (the `ERR_MODULE_NOT_FOUND`),
	// and not the `ERR_REQUIRE_ESM` error, which is a red herring.
	throw err;

I think ERR_REQUIRE_ESM is not thrown anymore (it's now a SyntaxError). Also, I don't really understand your logic for trying to handle it this way. AFAIK, Node.js (at least for modules) only throws ERR_MODULE_NOT_FOUND when something is actually not found; if that's not true, I think it's probably wrong and should be updated (if it is still the case, could you confirm and provide a stacktrace so I can fix it).

ERR_UNKNOWN_FILE_EXTENSION: require() will interpret any unknown file extension as commonjs, but it shouldn't be blindly applied without first checking whether format is known (for instance, packageJson.type = 'module' or a loader set the format to 'module'). So this condition should be paired with another condition, something like format !== 'module' && err.code === 'ERR_UNKNOWN_FILE_EXTENSION'.

[juergba]

@giltayar could you have a look at this one, please?

[JakobJingleheimer]

Oh, well hello @giltayar haha

[giltayar]

@juergba @JakobJingleheimer. I've seen this too! Let me look into it this week (or the week after).

[giltayar]

@JakobJingleheimer I tried reproducing the problem, and couldn't. My setup was Node and mocha versions exactly like yours,

$ npm install mocha babel-register-esm # babel-register-esm is a loader of mine that transpiles via babel
$ cat test.mjs
import '@testing-library/jest-dom';
$ npx mocha --loader=babel-register-esm ./test.mjs
(node:3252) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
(node:3252) DeprecationWarning: Obsolete loader hook(s) supplied and will be ignored: getFormat, transformSource

Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@testing-library/jest-dom' imported from /Users/giltayar/code/tmp/mocha-esm-problem/test.mjs
...

So it looks like it's working...? What did I miss here?

[JakobJingleheimer]

Your file extension is .mjs. That's why it's not reproducing for you 😉 Mocha has a specific check just above the code I cited before for .mjs (it returns before hitting the bugged code):

mocha/lib/nodejs/esm-utils.js

Lines 44 to 46 in 28b4824

	if (path.extname(file) === '.mjs') {
	return formattedImport(file);
	}

Ensure your file extension is not .mjs or .js.

$> NODE_ENV=test NODE_OPTIONS='--loader=./loader.mjs' npx mocha ./test/index.spec.jsx

/…/test/index.spec.jsx:1
import {
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at Object.compileFunction (node:vm:352:18)
    at wrapSafe (node:internal/modules/cjs/loader:1026:15)
    at Module._compile (node:internal/modules/cjs/loader:1061:27)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1149:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:999:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.exports.requireOrImport (/…/test/node_modules/mocha/lib/nodejs/esm-utils.js:56:20)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Object.exports.loadFilesAsync (/…/test/node_modules/mocha/lib/nodejs/esm-utils.js:88:20)
    at async singleRun (/…/test/node_modules/mocha/lib/cli/run-helpers.js:125:3)
    at async Object.exports.handler (/…/test/node_modules/mocha/lib/cli/run.js:374:5)

The contents of ./test/index.spec.jsx that produced the above is just:

import 'nonexistent';

If you replace that import with one to something that does exist, all is well.

[giltayar]

@JakobJingleheimer reproduced! The fix is really ugly, as this whole section of code is heuristics in figuring out whether the require threw for "real" reasons or just because it wasn't supposed to be require-ed.

But it's mostly passing the test of time, and I can't think of another way to fallback to require if import fails (for example, if there are require hooks that are needed).

Hmm... maybe we don't really need the require? is it time we got rid of it? AFAIR, require hooks are still called when importi-ng a CJS file.

(just thoughts. Will be fixed one way or another this weekend)

[JakobJingleheimer]

🤔 can you not trust ERR_MODULE_NOT_FOUND? If it's removed from the 3 conditions currently used to determine whether to try require(), Mocha will throw the true error.

Then in the try/catch for require(), check for a SyntaxError instead of the nonexistent ERR_REQUIRE_ESM.

I just tried both, and it worked for this bug's scenario:

Removing the ERR_MODULE_NOT_FOUND addresses the case where Node has already (correctly) determined the file doesn't exist.

Updating the check for ERR_REQUIRE_ESM → instanceof SyntaxError + selective requireErr.message substring match addresses where there wasn't enough info before to know whether require() is appropriate (so try and see, as Mocha already does), but it turns out, nope, not appropriate.

[giltayar]

@JakobJingleheimer unfortunately, I can't. The ERR_MODULE_NOT_FOUND condition is there because you can run mocha ./foo and assume that Mocha will load ./foo.js.

But I fixed the problem and a PR will be coming in a few minutes! (it's a hack, but the whole way of determining whether a file is ESM or CJS is ultimately a hack)