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.

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

Jump to bottom

rollup/commonjs plugin sometimes fails to detect circular dependency #1425

Closed
dskloetd opened this issue Jan 31, 2023 · 5 comments · May be fixed by #1639
Closed

rollup/commonjs plugin sometimes fails to detect circular dependency #1425

dskloetd opened this issue Jan 31, 2023 · 5 comments · May be fixed by #1639
Labels
x⁷ ⋅ stale

Comments

@dskloetd
Copy link

  • 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
This was referenced Mar 6, 2023
Closed
Merged
dskloetd added a commit to dfinity/nns-dapp that referenced this issue Mar 7, 2023
@dskloetd
bump agent-js to 0.15.4 (#2033)
b522b24 
# Motivation

The new agent-js release has fewer circular dependencies which helps
build reproducibility because of
rollup/plugins#1425.

# Changes

`npm run update:agent`

# Tests

Clicked around a bit on testnet.
@lukastaegert
Copy link
Member

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
Copy link
Author

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

@dskloetd dskloetd mentioned this issue Apr 18, 2023
Merged
github-merge-queue bot pushed a commit to dfinity/nns-dapp that referenced this issue Apr 19, 2023
@dskloetd
Potential work-around for Rollup cyclic dependency bug (#2332)
51e4e1f 
# Motivation

There is a bug in Rollup that non-deterministically fails to detect
cyclic
dependencies. rollup/plugins#1425
As a result `lru-cache` is sometimes wrapped in a `requireLruCache`
function and sometimes it is not.
This makes our build non-deterministic.
`lru-caceh` is not itself part of a circular dependency but
`semver/classes/range.js`, which depends on `lru-cache`, is.
By depending on `lru-cache` directly from our app, we change the order
in which modules are analyzed by Rollup, hopefully avoiding the buggy
behavior.
It probably doesn't matter where we put this import, so I put it as
close to the root of the app as I could find.

# Changes

Add `import "lru-cache"` in frontend/src/routes/+layout.ts

# Tests

Inspected generated `vendor` chunk to see that it does not have
`hasRequiredLruCache`.
`dfx deploy nns-dapp` locally and did some manual testing.
@peterpeterparker peterpeterparker mentioned this issue Apr 21, 2023
Closed
@stale stale bot added the x⁷ ⋅ stale label May 19, 2023
@stale
Copy link

stale bot commented May 22, 2023

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
Copy link
Author

disappointing

@stale stale bot removed the x⁷ ⋅ stale label May 22, 2023
@stale stale bot added the x⁷ ⋅ stale label Aug 10, 2023
@stale
Copy link

stale bot commented Aug 12, 2023

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

@stale stale bot closed this as completed Aug 12, 2023
This was referenced Oct 29, 2023
Open
Open
@bluwy bluwy mentioned this issue Nov 27, 2023
Open
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
x⁷ ⋅ stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(commonjs)!: default strictRequires to true bluwy/plugins
2 participants
@lukastaegert @dskloetd