Repository: mochajs/mocha

Problem statement:
Missing file/package in import statement misreported as ESM error
### Prerequisites

<!--
Place an `x` between the square brackets on the lines below for every satisfied prerequisite.
-->

- [x] Checked that your issue hasn't already been filed by cross-referencing [issues with the `faq` label](https://github.com/mochajs/mocha/issues?utf8=%E2%9C%93&q=is%3Aissue%20label%3Afaq%20)
- [x] 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.
- [x] '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
- [x] 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

<details>
<summary>test.jsx</summary>

Without installing `@testing-library/jest-dom`:

```js
import '@testing-library/jest-dom';
```
</details>

```console
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

<!-- If applicable, please specify: -->

- 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


Hints:
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:

https://github.com/mochajs/mocha/blob/28b482472a519b7abaf30a18b8ad709707bfd5a7/lib/nodejs/esm-utils.js#L58-L65

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'`.

@giltayar could you have a look at this one, please?
Oh, well hello @giltayar haha
@juergba @JakobJingleheimer. I've seen this too! Let me look into it this week (or the week after).
@JakobJingleheimer I tried reproducing the problem, and couldn't. My setup was Node and mocha versions exactly like yours,

```sh
$ 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?
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):

https://github.com/mochajs/mocha/blob/28b482472a519b7abaf30a18b8ad709707bfd5a7/lib/nodejs/esm-utils.js#L44-L46

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:

```mjs
import 'nonexistent';

```

If you replace that import with one to something that does exist, all is well.
@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)
🤔 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.
@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)


Patch:
diff --git a/lib/nodejs/esm-utils.js b/lib/nodejs/esm-utils.js
--- a/lib/nodejs/esm-utils.js
+++ b/lib/nodejs/esm-utils.js
@@ -53,15 +53,30 @@ exports.requireOrImport = hasStableEsmImplementation
           err.code === 'ERR_UNSUPPORTED_DIR_IMPORT'
         ) {
           try {
+            // Importing a file usually works, but the resolution of `import` is the ESM
+            // resolution algorithm, and not the CJS resolution algorithm. So in this case
+            // if we fail, we may have failed because we tried the ESM resolution and failed
+            // So we try to `require` it
             return require(file);
           } catch (requireErr) {
-            if (requireErr.code === 'ERR_REQUIRE_ESM') {
-              // This happens when the test file is a JS file, but via type:module is actually ESM,
+            if (
+              requireErr.code === 'ERR_REQUIRE_ESM' ||
+              (requireErr instanceof SyntaxError &&
+                requireErr
+                  .toString()
+                  .includes('Cannot use import statement outside a module'))
+            ) {
+              // ERR_REQUIRE_ESM 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,
+              // 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.
+              //
+              // SyntaxError happens when in an edge case: when we're using an ESM loader that loads
+              // a `test.ts` file (i.e. unrecognized extension), and that file includes an unknown
+              // import (which thows an ERR_MODULE_NOT_FOUND). require-ing it will throw the
+              // syntax error, because we cannot require a file that has import-s.
               throw err;
             } else {
               throw requireErr;


Test patch:
diff --git a/test/integration/esm.spec.js b/test/integration/esm.spec.js
--- a/test/integration/esm.spec.js
+++ b/test/integration/esm.spec.js
@@ -81,4 +81,25 @@ describe('esm', function () {
       'test-that-imports-non-existing-module'
     );
   });
+
+  it('should throw an ERR_MODULE_NOT_FOUND and not ERR_REQUIRE_ESM if file imports a non-existing module with a loader', async function () {
+    const fixture =
+      'esm/loader-with-module-not-found/test-that-imports-non-existing-module.fixture.ts';
+
+    const err = await runMochaAsync(
+      fixture,
+      [
+        '--unhandled-rejections=warn',
+        '--loader=./test/integration/fixtures/esm/loader-with-module-not-found/loader-that-recognizes-ts.mjs'
+      ],
+      {
+        stdio: 'pipe'
+      }
+    ).catch(err => err);
+
+    expect(err.output, 'to contain', 'ERR_MODULE_NOT_FOUND').and(
+      'to contain',
+      'non-existent-package'
+    );
+  });
 });
diff --git a/test/integration/fixtures/esm/loader-with-module-not-found/loader-that-recognizes-ts.mjs b/test/integration/fixtures/esm/loader-with-module-not-found/loader-that-recognizes-ts.mjs
new file mode 100644
--- /dev/null
+++ b/test/integration/fixtures/esm/loader-with-module-not-found/loader-that-recognizes-ts.mjs
@@ -0,0 +1,18 @@
+import path from 'path'
+import {fileURLToPath} from 'url'
+
+/**
+ * @param {string} specifier
+ * @param {{
+ *   conditions: !Array<string>,
+ *   parentURL: !(string | undefined),
+ * }} context
+ * @param {Function} defaultResolve
+ * @returns {Promise<{ url: string }>}
+ */
+export async function resolve(specifier, context, defaultResolve) {
+  const extension = path.extname(
+    fileURLToPath(/**@type {import('url').URL}*/ (new URL(specifier, context.parentURL))),
+  )
+  return await defaultResolve(specifier.replace('.ts', '.mjs'), context, defaultResolve)
+}
diff --git a/test/integration/fixtures/esm/loader-with-module-not-found/test-that-imports-non-existing-module.fixture.mjs b/test/integration/fixtures/esm/loader-with-module-not-found/test-that-imports-non-existing-module.fixture.mjs
new file mode 100644
--- /dev/null
+++ b/test/integration/fixtures/esm/loader-with-module-not-found/test-that-imports-non-existing-module.fixture.mjs
@@ -0,0 +1 @@
+import 'non-existent-package';
diff --git a/test/integration/fixtures/esm/loader-with-module-not-found/test-that-imports-non-existing-module.fixture.ts b/test/integration/fixtures/esm/loader-with-module-not-found/test-that-imports-non-existing-module.fixture.ts
new file mode 100644
--- /dev/null
+++ b/test/integration/fixtures/esm/loader-with-module-not-found/test-that-imports-non-existing-module.fixture.ts
@@ -0,0 +1,2 @@
+// This file will be resolved to `test-that-imports-non-existing-module.fixture.mjs` by the loader
+import 'non-existent-package';
diff --git a/test/integration/helpers.js b/test/integration/helpers.js
--- a/test/integration/helpers.js
+++ b/test/integration/helpers.js
@@ -368,7 +368,11 @@ function createSubprocess(args, done, opts = {}) {
  * @returns {string} Resolved filepath
  */
 function resolveFixturePath(fixture) {
-  if (path.extname(fixture) !== '.js' && path.extname(fixture) !== '.mjs') {
+  if (
+    path.extname(fixture) !== '.js' &&
+    path.extname(fixture) !== '.mjs' &&
+    path.extname(fixture) !== '.ts'
+  ) {
     fixture += '.fixture.js';
   }
   return path.isAbsolute(fixture)