Circular dependencies (prevents proper rollup)

Rollup spits those out:

Circular dependency: node_modules\readable-stream\lib\_stream_duplex.js -> node_modules\readable-stream\lib\_stream_readable.js -> node_modules\readable-stream\lib\_stream_duplex.js
Circular dependency: node_modules\readable-stream\lib\_stream_duplex.js -> node_modules\readable-stream\lib\_stream_readable.js ->  commonjs-proxy:C:\Users\Daniel.SILVERBOLT\Desktop\git\endpoint-analyzer\node_modules\readable-stream\lib\_stream_duplex.js -> nod
e_modules\readable-stream\lib\_stream_duplex.js
Circular dependency: node_modules\readable-stream\lib\_stream_writable.js -> node_modules\readable-stream\lib\_stream_duplex.js -> node_modules\readable-stream\lib\_stream_writable.js
Circular dependency: node_modules\readable-stream\lib\_stream_writable.js -> node_modules\readable-stream\lib\_stream_duplex.js ->  commonjs-proxy:C:\Users\Daniel.SILVERBOLT\Desktop\git\endpoint-analyzer\node_modules\readable-stream\lib\_stream_writable.js -> n
ode_modules\readable-stream\lib\_stream_writable.js

It is a huge problem, and will make code unexecutable after "rolling up".
The rollup process will convert commonjs deps into "static imports", which do not support circular dependencies.
Then it will end up with a flat hierarchy where there's a reference to a (yet) undefined variable.

Do you think this is a rollup problem or a readable-stream one? IMHO, it’s the former, as circular deps are valid cjs.

I’m happy to have a look anyway. Can you please produce a project/full example that demonstrate the issue?

It's kind of both. The problem is not really rollup, as what rollup does is "convert" (through a plugin) into plain ES static imports, which do not support circular dependencies.

Circular dependencies are valid in cjs - but are still problematic even there because you have to be really careful about what is really export/imported and when does the circular dependency break it.
It's really a design flaw- that could be easily fixed by lifting common code/data into separate js files.

I'll try to make up a sample project

Here is an example:

rollup-example.zip

You could run npm run build to see the build process, or just inspect the pre-compiled output at compiled.js.
Try to run it...
You'll get TypeError [ERR_INVALID_ARG_TYPE]: The "superCtor" argument must be of type Function. Received type undefined

Now look at line 1135 - you'll see the compiled util$1.inherits(Duplex$1, _stream_readable); where _stream_readable is declared only at line 1497 var _stream_readable = Readable;.
Hence _stream_readable is undefined and you get TypeError [ERR_INVALID_ARG_TYPE]:

As rollup is becoming very popular and big projects are moving there (in my opinion partly because browsers started supporting ES module system and there's much less need for transpilers), it becomes crucial to adjust modules to dump the circular dependencies, even if not adopting ES imports yet.

Just a note: circular dependencies are valid in ESM code. It's just that they behave differently compared to CJS because files are not executed in the same order.

This does not look like an easy case to fix:

_stream_duplex exports the Duplex class which uses Writable.
_stream_writable exports the Writable class which uses Duplex.

There is no common dependency that could be factored out.

Well Writable only imports Duplex in order to test if instanceof, I'm sure we can find another way to check if it's a Duplex :-)

Using webpacks circular dependency plugin also brings this up.

Instead of this:

function ReadableState(options, stream, isDuplex) {
  Duplex = Duplex || require('./_stream_duplex');
  options = options || {}; // Duplex streams are both readable and writable, but share
  // the same options object.
  // However, some cases require setting options to different
  // values for the readable and the writable sides of the duplex stream.
  // These options can be provided separately as readableXXX and writableXXX.

  if (typeof isDuplex !== 'boolean') isDuplex = stream instanceof Duplex; // object stream flag. Used to make read(n) ignore n and to

You could do this:

function ReadableState(options, stream, isDuplex) {
  options = options || {}; // Duplex streams are both readable and writable, but share
  // the same options object.
  // However, some cases require setting options to different
  // values for the readable and the writable sides of the duplex stream.
  // These options can be provided separately as readableXXX and writableXXX.

  if (typeof isDuplex !== 'boolean') {
    isDuplex = stream instanceof Readable && typeof stream._write === 'function' && typeof stream._writableState === 'object';
  }

With NodeJS streams, it's all about duck-typing. In order for something to be a stream it does not need to actually inherit from the base classes, it just needs to conform to a certain protocol that everyone expects.

Unfortunately we cannot do that, for two reasons:

this modules tries to follow what Node.js core does. If we would like to change that behavior, we should seek to change it there.
Unfortunately these lines are not about ducktyping, they mean exactly that specific function, because it comes from these source files.

I am aware that there IS support for circular dependencies, but not the way you do them :-)
You are dynamically requiring the Duplex module, because that's easier in CommonJS. But when converting to ES6, one needs to jump through hoop to get the same theoretical behavior as in CJS.
That creates a hot mess, and neither rollup nor webpack can handle that.
You could have gone the other way around, requiring them in the top scope, but then you have to not replace module.exports, instead you have to attach the exported stuff to it. This would be more consistent with how ES6 does that, but less consistent with (current) nodejs code.

So another compromise which would not change the almost-100% nodejs compatibility, would be a global (private) registry.

Something like this:

_registry.js:
// An empty file would also do as it has an empty `exports` object, but may behave differently when converted to ES6
const registry = {};
module.exports = registry;

_stream_duplex.js:

const Registry = require('_registry');
.
.
.
Registry.Duplex = Duplex;

_stream_readable.js:

const Registry = require('_registry');


function ReadableState(options, stream, isDuplex) {
// -  Duplex = Duplex || require('./_stream_duplex');
.
.
.
  if (typeof isDuplex !== 'boolean') isDuplex = stream instanceof Registry.Duplex; // object stream flag. Used to make read(n) ignore n and to
  // make all the buffer merging and length checks go away

.
.
.
}
.
.
.

function Readable(options) {
// -  Duplex = Duplex || require('./_stream_duplex');
.
.
.
  var isDuplex = this instanceof Registry.Duplex;
.
.
.
}

Registry.Readable = Readable;

Executing the Readable constructor will only happen after all classes registered, so everyone can access everyone.

Would you mind to send a PR with that implementation to https://github.com/nodejs/node? That is a change that we should aim to do there.

@mcollina

@mcollina I'm not really that this change is relevant to nodejs itself. This is because we never "bundle" node's internal modules.
I think that the "registry" would actually look weird in node's implementation as there's no rationale for it being there.
A better change in node's code would be to require them in the top level, and fix the export to not replace module.exports. (Or even to go ES6 but that's a much bigger project that will affect the whole core lib)

A better change in node's code would be to require them in the top level, and fix the export to not replace module.exports.

We can make that change in Node.js core, as it will simplify the bootstrap phase there as well. Would you like to work on that?

@mcollina

@mcollina Let me play a little bit with it in a playground, see how this behaves in different rollup/webpack configurations. I want to improve things, not break them :-)

Any update on this one?

@thekevinbrown

as vite-compatible-readable-stream fork from @thekevinbrown was causing compatiility issues in other modules, I went ahead with a much simpler patch:
gluck@8288836

Which I published here: https://www.npmjs.com/package/readable-stream-no-circular

I didn't fix the tests which are directly importing _stream_*.js files (hence Readable.Duplex isn't available at runtime), but I don't think such usage is supported.
I'm sure I missed something, but that works for me.

@gluck

@gluck that would be installed with "readable-stream": "npm@readable-stream-no-circular" ?

@gluck

@gluck that would be installed with "readable-stream": "npm@readable-stream-no-circular" ?

I think npm:readable-stream-no-circular is the correct syntax, but yes (though I'm aliasing it with a rollup alias at bundling time myself)

For what it's worth vite-compatible-readable-stream should be just readable stream with the circular dependencies removed. There's another package that patches React PDF but that's separate from what I set up.

@thekevinbrown

Thank you @thekevinbrown ! Adding an alias to readable-stream on Vite configuration with your package fixed my issue 😄

I ran yarn install vite-compatible-readable-stream, then on vite.config.ts:

import { defineConfig } from "vite";

export default defineConfig({
  ...
  resolve: {
    alias: {
      "readable-stream": "vite-compatible-readable-stream",
    }
  }
  ...
});

BTW my issue came from needing to run simple-peer within a Vite app

I just wanted to add that I've had issues in my CI pipeline with semantic-release after installing the vite-compatible-readable-stream.

Here is how I adjusted in .travis.yml:

after_success:
  # we are aliasing readable-stream to https://github.com/exogee-technology/readable-stream
  # however this breaks semantic release because the shape of the export is different
  # so we install the regular version of readable stream to enable expected use by semantic release
  # it is an inelegant patch but it's a quick solution to the problem and is acceptable as it is the last step of the CI
  - npm uninstall readable-stream
  - npm install readable-stream
  - npm run semantic-release

If that's helpful to anyone

build(em): add `vite` as an option

If one is using pnpm & rollup without vite, one can use pnpm's alias...https://pnpm.io/aliases

However, it would be great to have the circular dependency fixed in this package.

fix: use vite compatible readable stream to fix torus auth

If your code relies on circular dependencies, you can suppress the warnings using onwarn, which doesn't seem to be properly documented:

// rollup.config.js
export default [
  {
    // ...
    onwarn: (warning, defaultHandler) => {
      if (warning.code === "CIRCULAR_DEPENDENCY") {
        return;
      }
      defaultHandler(warning);
    },
  },
];

build: update Rollup and include archiver in build

Navigation Menu

Repository navigation

Circular dependencies (prevents proper `rollup`) #348

28 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Circular dependencies (prevents proper rollup) #348

Description

Activity

mcollina commented on Jul 16, 2018

danielgindi commented on Jul 16, 2018

danielgindi commented on Jul 16, 2018

targos commented on Jul 16, 2018

targos commented on Jul 16, 2018

danielgindi commented on Jul 16, 2018

FabianSellmann commented on Mar 18, 2019

danielgindi commented on Mar 18, 2019

mcollina commented on Mar 18, 2019

danielgindi commented on Mar 18, 2019

mcollina commented on Mar 18, 2019

danielgindi commented on Mar 18, 2019

mcollina commented on Mar 18, 2019

danielgindi commented on Mar 18, 2019

manucorporat commented on Jun 18, 2019

28 remaining items

gluck commented on Nov 19, 2021

y-nk commented on Nov 22, 2021

gluck commented on Nov 22, 2021

thekevinbrown commented on Nov 22, 2021

brunodb3 commented on Apr 12, 2022

lemoustachiste commented on Apr 12, 2022

btakita commented on Sep 10, 2022

WesSouza commented on Dec 31, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Circular dependencies (prevents proper `rollup`) #348