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

Fix circular dependency in Node polyfills #36

Closed
wants to merge 1 commit into from

Conversation

eigendude
Copy link

@eigendude eigendude commented May 30, 2022

Description

This PR contains the fix in nodejs/readable-stream#458 hand-applied to the several polyfills in this repo. From the other PR's 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.

How has this been tested?

I applied the patch to my Snowpack project after installing npm modules. I added the following to my snowpack.config.cjs file:

const polyfills = require("rollup-plugin-node-polyfills");

module.exports = {
  ...
  packageOptions: {
    rollup: {
      plugins: [
        ...
        polyfills(),
        ...
      ],
    },
  },
};

I started the live server. No errors observed.

Related PRs

See nodejs/readable-stream#458

At the time `inherits(Duplex, Readable);` is called, Readable is
hoisted but not yet defined due to a circular dependency. Fix this
by removing the circular depedency.
@eigendude eigendude changed the title Fix circular dependency in Node polyfill Fix circular dependency in Node polyfills May 30, 2022
@eigendude
Copy link
Author

eigendude commented May 20, 2024

The Rollup ecosystem has advanced considerably from the last two years, seeing the deprecation of Snowpack and the broader adoption of Vite. As such, these polyfills are unlikely to be needed going forward.

@eigendude eigendude closed this May 20, 2024
@eigendude eigendude deleted the fix-circular branch May 20, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant