fix: wrong error thrown if loader is used (#4807)

Author: giltayar

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/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'
+    );
+  });
 });

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)
+}

test/integration/fixtures/esm/loader-with-module-not-found/test-that-imports-non-existing-module.fixture.mjs

@@ -0,0 +1 @@
+import 'non-existent-package';

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';

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)