Fix circular dependency #458
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eigendude
force-pushed
the
fix-circular
branch
from
206ef41
to
fee86c7
Compare
At the time `require('inherits')(Duplex, Readable)` is called,
Readable is hoisted but not yet defined due to a circular dependency.
Because the only uses of Duplex by the base classes are checking for
if they themselves are a Duplex, we can replace typeof with a boolean.
This boolean actually already exists in Writable, so it makes sense
to use it elsewhere to solve the circular dependency.
eigendude
force-pushed
the
fix-circular
branch
from
fee86c7
to
804eaab
Compare
|
Is there any way to get this reviewed? Thanks. |
|
@mcollina hi! |
|
Is there a reason for the closure? |
|
It's fixed in the upcoming v4 release. |
|
Nice!!! Mind linking to the commit? Also, I ported this patch to rollup-plugin-node-polyfills: ionic-team/rollup-plugin-node-polyfills#36 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes a circular dependency that has been affecting the rollup ecosystem for a long time. The fix is simple, as the circularity comes from a single type test that can be simply replaced with a boolean. (And the boolean already exists, it's just exposed to the Duplex child class now).
The root cause is due to rollup's handling of hoisting. Duplex inherits from Readable and Writable, but Readable and Writable check for Duplex-ity, ergo circular. As a result, when Duplex inherits from Readable/Writable, these two classes are hoisted but not yet defined.
The fix is simple: Replace the Duplex-ity test with a boolean, set by Duplex when it constructs its base classes.
This boolean actually already exists locally in both Readable and Writable, so it makes sense to use it to bust the circular dependency.
Related PRs
Closes #348
Closes calvinmetcalf/rollup-plugin-node-builtins#31