From baa12fd73e59ab6139d05d5eb76222c5d7a774ba Mon Sep 17 00:00:00 2001 From: Gil Tayar Date: Mon, 10 Jan 2022 18:45:59 +0200 Subject: [PATCH] fix: wrong error thrown if loader is used (#4807) --- lib/nodejs/esm-utils.js | 21 ++++++++++++++++--- test/integration/esm.spec.js | 21 +++++++++++++++++++ .../loader-that-recognizes-ts.mjs | 18 ++++++++++++++++ ...at-imports-non-existing-module.fixture.mjs | 1 + ...hat-imports-non-existing-module.fixture.ts | 2 ++ test/integration/helpers.js | 6 +++++- 6 files changed, 65 insertions(+), 4 deletions(-) create mode 100644 test/integration/fixtures/esm/loader-with-module-not-found/loader-that-recognizes-ts.mjs create mode 100644 test/integration/fixtures/esm/loader-with-module-not-found/test-that-imports-non-existing-module.fixture.mjs create mode 100644 test/integration/fixtures/esm/loader-with-module-not-found/test-that-imports-non-existing-module.fixture.ts diff --git a/lib/nodejs/esm-utils.js b/lib/nodejs/esm-utils.js index fd51e012cc..9a854be376 100644 --- 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; diff --git a/test/integration/esm.spec.js b/test/integration/esm.spec.js index 8935214aaf..3209a2ce38 100644 --- 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 index 0000000000..a233fee830 --- /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, + * 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 index 0000000000..63425cf618 --- /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 index 0000000000..22db1d5ed9 --- /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 index 6f16bd72b6..3668089e2c 100644 --- 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)