[v20.6] CJS runs repeatedly when there are circular dependencies when loaded by ESM

Version

v20.6.0

Platform

all

Subsystem

No response

What steps will reproduce the bug?

https://github.com/liuxingbaoyu/node-v20.6-bug

How often does it reproduce? Is there a required condition?

always

What is the expected behavior? Why is that the expected behavior?

node .\main.js
run
success

What do you see instead?

node .\main.js
run
run
success

Additional information

This should be the regression introduced by v20.6.

Note that this is breaking@babel/core, and thus all the frameworks/project that use it. It would be great to prioritize it 🙏

Can someone give a reproducible example? The code from https://github.com/liuxingbaoyu/node-v20.6-bug doesn't show the problem (it throws an error in mod/index.js).

Sorry, I forgot to push the second commit.
But the initial error is also different due to the regression.

Please try again, thanks!

Right, that error happens because it runs twice. You can replace index.js with the following to not see the error:

console.log("RUNNING");
exports.File = 2;
require("./1");

A minimal reproduction:

// index.mjs
import "./dep.cjs";

// dep.cjs
require("./dep.cjs");
console.log("RUNNING");

Running node index.mjs logs twice but it should log once.

I have a repro. Starting to bisect.

@targos If it helps, it looks like the problem is that ESM's CJS loader doesn't set this loaded = true before evaluating the CJS module.

Here probably:

node/lib/internal/modules/esm/translators.js

Line 326 in 5b31ff1

cjsParseCache.set(module, { source, exportNames });

Bug is introduced by #47999

In case there is no easy fix, would it be possible to revert that PR?

(cc @aduh95)

Thanks for the ping, I'll have a look

To help with search, this is the issue for the error "TypeError: Cannot redefine property: File" generated in @babel/core. See babel/babel#15927

Assuming angular tests would have caught this, Should we add it to citgm? (The failure seem to break angular)

angular/angular-cli#25782

In case there is no easy fix, would it be possible to revert that PR?

(cc @aduh95)

The change from #47999 is opt-in. Why not just stop opting in?

@rluvaton We are also working on nodejs/citgm#628 (angular was broken through Babel)

angular was broken through Babel

Same thing with astro: withastro/astro #8411

@rluvaton We are also working on nodejs/citgm#628 (angular was broken through Babel)

Jest (which is in CITGM already) also uses Babel, I'm surprised that wasn't discovered...

@SimenB This bug is only triggered when the ESM package uses a CJS package. :)

Jest's CI is failing now, I'd have thought those would have been caught.

EDIT: right, it's the build script (which is in ESM): https://github.com/jestjs/jest/actions/runs/6094016439/job/16535303696

CITGM result from v20.6.0 release: #49185 (comment). Apparently, jest broke but no one looked at these failures (that's why I suggested: nodejs/citgm#955)

Stack: https://ci.nodejs.org/job/citgm-smoker/3212/nodes=fedora-last-latest-x64/testReport/junit/(root)/citgm/jest_v29_6_4/

Right, so there at least the error reported in this bug can be seen.

If Jest is known to be flaky on CITGM, I'd be happy to take a look. I don't think I've been pinged on any CITGM stuff since v19 release (#44626 (comment))

When could we expect a release for the fix that was merged ? 🙏

For people watching issue this for the error with Babel: we added a workaround in @babel/core, @babel/traverse and @babel/types 7.22.17. Please make sure to upgrade and they should be compatible with Node.js 20.6.0.

Anyone with a Quick fix, or which version is stable?
My version: v20.6.0

v20.6.1 is stable.