fix: resolve ERR_REQUIRE_CYCLE_MODULE for never installed modules #5294

Donbasilpeter · 2025-02-06T10:04:30Z

PR Checklist

Addresses an existing open issue: fixes Misleading ERR_REQUIRE_CYCLE_MODULE when attempting to import from module if module was never installed #5290
That issue was marked as status: accepting prs
Steps in CONTRIBUTING.md were taken

Overview

The requireOrImport function was originally set up to try importing a file as an ES module first, and if that failed, it would fall back to require. However, since Node.js version 22.12, the require function now automatically handles both ES modules (ESM) and CommonJS (CJS). This caused an issue where calling require after a failed import could lead to a cyclic dependency error.

in the updated code I have used require first and if fails with requireErr.code === 'ERR_REQUIRE_ESM' which only happens if version is <22.12.0 and and target file is esm ,then use the older method.

https://nodejs.org/en/blog/release/v22.12.0

https://joyeecheung.github.io/blog/2024/03/18/require-esm-in-node-js

…chajs#5290) Use require to handle both ESM and CJS in the latest Node.js due to loadESMFromCJS function.

linux-foundation-easycla · 2025-02-06T10:04:34Z

The committers listed above are authorized under a signed CLA.

✅ login: Donbasilpeter / name: Don Basil Peter (3e476fe, 982a7c8)

JoshuaKGoldberg

Thanks for getting this started! 🚀

JoshuaKGoldberg · 2025-02-06T12:08:42Z

lib/nodejs/esm-utils.js

    }
  }
 };

+


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

Suggested change

lib/nodejs/esm-utils.js

Donbasilpeter · 2025-02-06T21:42:04Z

@JoshuaKGoldberg I have looked in to the test cases.

The only test case that fails :
https://github.com/mochajs/mocha/blob/main/test/integration/plugins/root-hooks.spec.js#L138

node js consider this as Ambiguous file but from versions >=node 2.7 this is not the case.

https://nodejs.org/en/blog/release/v22.7.0#module-syntax-detection-is-now-enabled-by-default

the function doesn't handle this case and returns an ERR_REQUIRE_CYCLE_MODULE Error,

the test case just expects this error and passes. but it is not the actual expected behaviour.

i jsut did a camparison which can be found below :

https://docs.google.com/spreadsheets/d/1T9AsIWK5tLsnqicjGuh9miyPGetemOgxLzGzAK3ZBJA/edit?usp=sharing

please review and feedback so that I can start updating the test case

JoshuaKGoldberg · 2025-02-07T14:02:23Z

😄 It's been a while since I've seen test cases sent in a spreadsheet. Those seem reasonable, thanks for thinking so deeply on them. It's hard to say for sure until we see them implemented in PR review though.

…hey are no longer considered ambiguous after Node.js 22.7.

Donbasilpeter · 2025-02-10T00:25:42Z

I also wanted to add that the test case to produce the specific issue #5920 is already present in the code:

https://github.com/mochajs/mocha/blob/main/test/integration/esm.spec.js

It can be reproduced by running with v22.12.0

JoshuaKGoldberg

Thanks for continuing on this! I have some questions.

JoshuaKGoldberg · 2025-02-16T21:42:24Z

lib/nodejs/esm-utils.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, 


[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,

JoshuaKGoldberg · 2025-02-16T21:46:40Z

lib/nodejs/esm-utils.js

+      // the code will import the module without any issues. For older versions, 
+      // this `if` statement is required to properly handle ESM modules.


[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.

JoshuaKGoldberg · 2025-02-16T21:49:01Z

lib/nodejs/esm-utils.js

+      // This `if` statement can be removed once all Node.js environments with current 
+      // support include the `loadESMFromCJS` function.


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.

JoshuaKGoldberg · 2025-02-16T21:50:51Z

lib/nodejs/esm-utils.js

+      (requireErr instanceof SyntaxError &&
+        requireErr
+          .toString()
+          .includes('Cannot use import statement outside a module'))


[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.

JoshuaKGoldberg · 2025-02-16T21:51:18Z

lib/nodejs/esm-utils.js

+      } catch (err) {
+        throw err;
      }


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

JoshuaKGoldberg · 2025-02-16T21:57:42Z

test/integration/plugins/root-hooks.spec.js

-        const noDetectModuleRegex = /SyntaxError: Unexpected token/;
-        const detectModuleRegex = /Cannot require\(\) ES Module/;
-
-        it('with --no-experimental-detect-module', function () {


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

JoshuaKGoldberg · 2025-02-16T21:58:34Z

lib/nodejs/esm-utils.js

-      err.code === 'ERR_MODULE_NOT_FOUND' ||
-      err.code === 'ERR_UNKNOWN_FILE_EXTENSION' ||
-      err.code === 'ERR_UNSUPPORTED_DIR_IMPORT'


[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.

Fix: Resolve ERR_REQUIRE_CYCLE_MODULE for never installed modules (mo…

3e476fe

…chajs#5290) Use require to handle both ESM and CJS in the latest Node.js due to loadESMFromCJS function.

JoshuaKGoldberg requested changes Feb 6, 2025

View reviewed changes

JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Feb 6, 2025

fix: update test case to handle ESM files without type=module since t…

982a7c8

…hey are no longer considered ambiguous after Node.js 22.7.

Donbasilpeter requested a review from JoshuaKGoldberg February 7, 2025 17:57

Donbasilpeter changed the title ~~Fix: Resolve ERR_REQUIRE_CYCLE_MODULE for never installed modules (#5290)~~ Fix: Resolve ERR_REQUIRE_CYCLE_MODULE for never installed modules Feb 10, 2025

JoshuaKGoldberg requested changes Feb 16, 2025

View reviewed changes

JoshuaKGoldberg changed the title ~~Fix: Resolve ERR_REQUIRE_CYCLE_MODULE for never installed modules~~ Fix: resolve ERR_REQUIRE_CYCLE_MODULE for never installed modules Feb 16, 2025

JoshuaKGoldberg changed the title ~~Fix: resolve ERR_REQUIRE_CYCLE_MODULE for never installed modules~~ fix: resolve ERR_REQUIRE_CYCLE_MODULE for never installed modules Feb 16, 2025

joyeecheung mentioned this pull request Feb 22, 2025

Mocha ERR_REQUIRE_CYCLE_MODULE on latest CJS nodejs/node#57172

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: resolve ERR_REQUIRE_CYCLE_MODULE for never installed modules #5294

fix: resolve ERR_REQUIRE_CYCLE_MODULE for never installed modules #5294

Donbasilpeter commented Feb 6, 2025 •

edited

Loading

linux-foundation-easycla bot commented Feb 6, 2025 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg Feb 6, 2025

Donbasilpeter commented Feb 6, 2025 •

edited

Loading

JoshuaKGoldberg commented Feb 7, 2025

Donbasilpeter commented Feb 10, 2025 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg Feb 16, 2025

JoshuaKGoldberg Feb 16, 2025

JoshuaKGoldberg Feb 16, 2025

JoshuaKGoldberg Feb 16, 2025

JoshuaKGoldberg Feb 16, 2025

JoshuaKGoldberg Feb 16, 2025

JoshuaKGoldberg Feb 16, 2025

		// the code will import the module without any issues. For older versions,
		// this `if` statement is required to properly handle ESM modules.

		// This `if` statement can be removed once all Node.js environments with current
		// support include the `loadESMFromCJS` function.

fix: resolve ERR_REQUIRE_CYCLE_MODULE for never installed modules #5294

Are you sure you want to change the base?

fix: resolve ERR_REQUIRE_CYCLE_MODULE for never installed modules #5294

Conversation

Donbasilpeter commented Feb 6, 2025 • edited Loading

PR Checklist

Overview

linux-foundation-easycla bot commented Feb 6, 2025 • edited Loading

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Donbasilpeter commented Feb 6, 2025 • edited Loading

JoshuaKGoldberg commented Feb 7, 2025

Donbasilpeter commented Feb 10, 2025 • edited Loading

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Donbasilpeter commented Feb 6, 2025 •

edited

Loading

linux-foundation-easycla bot commented Feb 6, 2025 •

edited

Loading

Donbasilpeter commented Feb 6, 2025 •

edited

Loading

Donbasilpeter commented Feb 10, 2025 •

edited

Loading