rollup/commonjs plugin sometimes fails to detect circular dependency

[dskloetd]

• Rollup Plugin Name: rollup/plugin-commonjs
• Rollup Plugin Version: 24.0.0
• Rollup Version: 3.12.0
• Operating System (or Browser): MacOS 13.2 (or Chrome 109.0.5414.119 (Official Build) (arm64) )
• Node Version: 18.13.0
• Link to reproduction (⚠️ read below): https://github.com/dskloetd/rollup-bug

Expected Behavior

All modules with circular dependencies should be wrapped in a "require wrapper".

Actual Behavior

The CommonJS plugin in rollup has a bug where it sometimes fails to detect a
circular dependency. The issue that this call to isCyclic
happens before the dependency graph is fully built and so
loadModule(resolved) might be skipped while it shouldn't be.
And then getTypeForFullyAnalyzedModule
here
is called before the module is indeed fully analyzed. Then the module doesn't
get a "require wrapper" while it does need one.

Additional Information

[lukastaegert]

There are indeed some problematic edge cases in the logic. Even worse, it can be that depending on race conditions in the module resolution, the output can become non-deterministic with regard to which modules are wrapped.
For that reason (and because the logic is really complicated) I am considering removing that logic again and instead to make strictRequires: true the default, with the option to manually opt out of this behavior for some or all modules.

[dskloetd]

Non-deterministic builds is indeed how I discovered the issue in the first place.

[stale]

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it. ⓘ

[dskloetd]

disappointing

[stale]

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it. ⓘ