Skip to content

fix: resolve ERR_REQUIRE_CYCLE_MODULE for never installed modules #5294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

fix: resolve ERR_REQUIRE_CYCLE_MODULE for never installed modules #5294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e476fe
Fix: Resolve ERR_REQUIRE_CYCLE_MODULE for never installed modules (#5…
Donbasilpeter Feb 6, 2025
982a7c8
fix: update test case to handle ESM files without type=module since t…
Donbasilpeter Feb 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 19 additions & 33 deletions lib/nodejs/esm-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,48 +39,34 @@ exports.requireOrImport = async (file, esmDecorator) => {
return formattedImport(file, esmDecorator);
}
try {
return dealWithExports(await formattedImport(file, esmDecorator));
} catch (err) {
return require(file);
} catch (requireErr) {
if (
err.code === 'ERR_MODULE_NOT_FOUND' ||
err.code === 'ERR_UNKNOWN_FILE_EXTENSION' ||
err.code === 'ERR_UNSUPPORTED_DIR_IMPORT'
Comment on lines -45 to -47
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Testing] This code explicitly handles modules not being found, unknown file extensions, and unsupported ESM directory import errors. For each of those three codes, could you please either reply with the existing test or add a new one? I just want to make sure we're on the same page of what test coverage there is.

requireErr.code === 'ERR_REQUIRE_ESM' || requireErr.code === 'ERR_INTERNAL_ASSERTION' ||
(requireErr instanceof SyntaxError &&
requireErr
.toString()
.includes('Cannot use import statement outside a module'))
Comment on lines +46 to +49
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question] Why not go off of requireErr.code the way the other checks do?

If there is some reason why it should go off other properties in the error, I'd think checking just requireErr.message would be preferable to stringifying the whole thing. In theory someone could name a file test/Cannot use import statement outside a module.just-to-be-annoying.spec.js.

) {
// In Node.js environments after version 22.11, the `loadESMFromCJS` function
// is used within the `require` function to handle cases where the target file
// is in ESM (ECMAScript Module) format. If the Node.js version is after 22.11,
Comment on lines +51 to +53
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Docs] loadESMFromCJS is an internal implementation of Node.js. Nothing would stop them from renaming it or refactoring to something else. We in userland don't really care about it anyway; we just care about the behavior of Node.js across versions:

Suggested change
// In Node.js environments after version 22.11, the `loadESMFromCJS` function
// is used within the `require` function to handle cases where the target file
// is in ESM (ECMAScript Module) format. If the Node.js version is after 22.11,
// When trying to require() an ESM module in Node.js >= 22.11,

// the code will import the module without any issues. For older versions,
// this `if` statement is required to properly handle ESM modules.
Comment on lines +54 to +55
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Docs] Phrases like "required to properly handle ..." are often indications the docs are unclear. The fact that the code's author believed it required to handle something is implied by the code existing. What readers really need to know is why it's required.

Suggested change
// the code will import the module without any issues. For older versions,
// this `if` statement is required to properly handle ESM modules.
// the code will import the module without any issues.
// For older Node.js versions, we fall back to trying an ESM `import()`.
// Note that the `import()` must come after the `require()` to avoid a cyclic dependency.
// See https://github.com/mochajs/mocha/issues/5290.

// This `if` statement can be removed once all Node.js environments with current
// support include the `loadESMFromCJS` function.
Comment on lines +56 to +57
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// This `if` statement can be removed once all Node.js environments with current
// support include the `loadESMFromCJS` function.
// This `if` statement can be removed once Mocha only supports Node.js versions
// that allow require(ESM): Node.js ^20.19.0 || >=22.12.0

That 20.19.0 is a guess, the wonderful nodejs/node#56927 hasn't been released yet.

try {
// Importing a file usually works, but the resolution of `import` is the ESM
// resolution algorithm, and not the CJS resolution algorithm. We may have
// failed because we tried the ESM resolution, so we try to `require` it.
return require(file);
} catch (requireErr) {
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,
// 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 throws 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;
}
return dealWithExports(await formattedImport(file, esmDecorator));
} catch (err) {
throw err;
}
Comment on lines +60 to 62
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question] Why a catch(err) that only has a throw err?

} else {
throw err;
throw requireErr;
}
}
};


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Style] Nit: unrelated change, let's revert.

Suggested change

function dealWithExports(module) {
if (module.default) {
return module.default;
Expand Down
47 changes: 19 additions & 28 deletions test/integration/plugins/root-hooks.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,47 +136,38 @@ describe('root hooks', function () {
});

describe('support ESM via .js extension w/o type=module', function () {
describe('should fail due to ambiguous file type', function () {
const filename =
'../fixtures/plugins/root-hooks/root-hook-defs-esm-broken.fixture.js';
const noDetectModuleRegex = /SyntaxError: Unexpected token/;
const detectModuleRegex = /Cannot require\(\) ES Module/;

it('with --no-experimental-detect-module', function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Testing] Why remove the --no-experimental-detect-module tests? That's still something we'll have to support.

const filename =
'../fixtures/plugins/root-hooks/root-hook-defs-esm-broken.fixture.js';
const [major, minor] = process.version.slice(1).split('.').map(Number);
if (major > 22 || (major === 22 && minor >= 7)) {
it('It should not fail since nodejs can recognize the file based on syntax', function () {
return expect(
invokeMochaAsync(
[
'--require=' + require.resolve(filename), // as object
'--no-experimental-detect-module'
],
'pipe'
)[1],
'when fulfilled',
'to contain output',
noDetectModuleRegex
runMochaForHookOutput([
'--require=' + require.resolve(filename),
require.resolve(
'../fixtures/plugins/root-hooks/root-hook-test.fixture.js'
)
]),
'to be fulfilled with',
['mjs afterAll', 'mjs beforeAll']
);
});

it('with --experimental-detect-module', function () {
// --experimental-detect-module was introduced in Node 21.1.0
const expectedRegex =
process.version >= 'v21.1.0'
? detectModuleRegex
: noDetectModuleRegex;
} else {
const noModuleDetectedRegex = /SyntaxError: Unexpected token/;
it('should fail due to ambiguous file type', function () {
return expect(
invokeMochaAsync(
[
'--require=' + require.resolve(filename), // as object
'--experimental-detect-module'
'--require=' + require.resolve(filename) // as object
],
'pipe'
)[1],
'when fulfilled',
'to contain output',
expectedRegex
noModuleDetectedRegex
);
});
});
}
});
});

Expand Down
Loading