Skip to content
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.

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

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

Closed
liuxingbaoyu opened this issue Sep 5, 2023 · 25 comments · Fixed by #49500
Closed
Labels
confirmed-bug Issues with confirmed bugs. esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem.

Comments

@liuxingbaoyu
Copy link
Contributor

liuxingbaoyu commented Sep 5, 2023

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.

@liuxingbaoyu liuxingbaoyu changed the title [v20.6] CJS cycle dependency throwing error is loaded by ESM [v20.6] CJS cycle dependencies throw an error when being loaded by ESM Sep 5, 2023
@liuxingbaoyu liuxingbaoyu changed the title [v20.6] CJS cycle dependencies throw an error when being loaded by ESM [v20.6] CJS runs repeatedly when there are circular dependencies when loaded by ESM Sep 5, 2023
kunish added a commit to MetaCubeX/metacubexd that referenced this issue Sep 5, 2023
matz3 added a commit to SAP/ui5-cli that referenced this issue Sep 5, 2023
There are issues with the current Node 20.6.0 version.
See: nodejs/node#49497
@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Sep 5, 2023

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

@targos
Copy link
Member

targos commented Sep 5, 2023

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

@liuxingbaoyu
Copy link
Contributor Author

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

@liuxingbaoyu
Copy link
Contributor Author

Please try again, thanks!

@nicolo-ribaudo

This comment was marked as outdated.

@nicolo-ribaudo
Copy link
Contributor

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.

@targos
Copy link
Member

targos commented Sep 5, 2023

I have a repro. Starting to bisect.

matz3 added a commit to SAP/ui5-project that referenced this issue Sep 5, 2023
There are issues with the current Node 20.6.0 version.
See: nodejs/node#49497
matz3 added a commit to SAP/ui5-project that referenced this issue Sep 5, 2023
There are issues with the current Node 20.6.0 version.
See: nodejs/node#49497
@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Sep 5, 2023

@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:

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

@targos
Copy link
Member

targos commented Sep 5, 2023

Bug is introduced by #47999

flovogt added a commit to SAP/ui5-logger that referenced this issue Sep 5, 2023
There are issues with the current Node 20.6.0 version.
See: nodejs/node#49497
flovogt added a commit to SAP/ui5-logger that referenced this issue Sep 5, 2023
There are issues with the current Node 20.6.0 version.
See: nodejs/node#49497
matz3 added a commit to SAP/ui5-cli that referenced this issue Sep 5, 2023
There are issues with the current Node 20.6.0 version.
See: nodejs/node#49497
matz3 added a commit to SAP/ui5-cli that referenced this issue Sep 5, 2023
There are issues with the current Node 20.6.0 version.
See: nodejs/node#49497
@nicolo-ribaudo
Copy link
Contributor

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

(cc @aduh95)

@aduh95
Copy link
Contributor

aduh95 commented Sep 5, 2023

Thanks for the ping, I'll have a look

@nicolo-ribaudo
Copy link
Contributor

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.

fisker added a commit to prettier/prettier that referenced this issue Sep 9, 2023
nodejs/node#49497 has been fixed and released.
mxschmitt added a commit to mxschmitt/playwright that referenced this issue Sep 9, 2023
lancetipton added a commit to lancetipton/parkin that referenced this issue Sep 12, 2023
* My guess is, it's related to this => nodejs/node#49497
* But could be something else. Seems to only happen in 20.6.0
travi added a commit to semantic-release/semantic-release that referenced this issue Sep 16, 2023
…he v20 range to v20.6.1

because of a known issue with v20.6.0 that we might as well avoid since we are restricting the
supported ranges at this point anyway: nodejs/node#49497

BREAKING CHANGE: the minimum supported version for the v20 range of node has been raised slightly to
v20.6.1 to avoid a known node bug
@Qodestackr
Copy link

Qodestackr commented Sep 18, 2023

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

@meyfa
Copy link
Contributor

meyfa commented Sep 18, 2023

v20.6.1 is stable.

Germandrummer92 pushed a commit to OctoMind-dev/playwright that referenced this issue Oct 27, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
PR-URL: nodejs#49500
Fixes: nodejs#49497
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
bjoluc added a commit to jspsych/jsPsych that referenced this issue Nov 8, 2023
riddla pushed a commit to tricks-gmbh/nuxt-image that referenced this issue Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.