rollup/commonjs plugin sometimes fails to detect circular dependency #1425
Comments
# 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.
|
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. |
|
Non-deterministic builds is indeed how I discovered the issue in the first place. |
# 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.
|
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. ⓘ |
|
disappointing |
|
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. ⓘ |
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
getTypeForFullyAnalyzedModulehere
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
The text was updated successfully, but these errors were encountered: