Circular dependencies (prevents proper rollup)

[danielgindi]

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.

[mcollina]

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?

[danielgindi]

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

[danielgindi]

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.

[targos]

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.

[targos]

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.

[danielgindi]

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 :-)

[FabianSellmann]

Using webpacks circular dependency plugin also brings this up.

[danielgindi]

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.

[mcollina]

Unfortunately we cannot do that, for two reasons:

1. 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.
2. Unfortunately these lines are not about ducktyping, they mean exactly that specific function, because it comes from these source files.

[danielgindi]

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.

[mcollina]

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.

[danielgindi]

@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)

[mcollina]

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?

[danielgindi]

@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 :-)

[manucorporat]

Any update on this one?

[mcollina]

No progress I'm afraid. Would you like to work on this?

[cdata]

I ran into this problem and have been catching up on the thread. @mcollina I'm willing to investigate a proposal to change Node.js, but I'm interested to hear from you: if such a change landed in Node, what would the process look like to land a similar change here?

[mcollina]

readable-stream@3 is extracted from the latest Node 10 release. A fix would have to land in Node master, then released in node 12. After that it’s minimum 2 weeks before it hits Node 10.

[cdata]

Thanks for those details. I'm compiling an issue to present the case for a change in the Node.js project.

[aurelhann]

@danielgindi do you find a workaround for rollup/webpack settings?
Thx to @cdata for the issue 👍

[brettz9]

Thought the issue should be tied to rollup/rollup#1507 , particularly the workaround in this comment (which has helped our indexeddbshim project be able to switch from browserify+babelify to Rollup).

[danielgindi]

Actually this looks good now in Node's core. They took the idea outlined here and implemented it, with Stream being the registry for types.

It still has a circular dependency, because they did not extract the Stream classes to be registered first under a different object - but it needs to be tested with rollup, as the Stream object is exported early and it may work.

[thekevinbrown]

I needed this to work in Vite and didn't have time to wait for the node change to land, so I applied the Registry pattern mentioned above on a fork here: https://github.com/TechInSite/readable-stream#readme

And I published it to: https://www.npmjs.com/package/vite-compatible-readable-stream

I did not do any testing beyond using this in Vite with React PDF, but it works there, so if it helps you then 👍 .

[lemoustachiste]

thanks @thekevinbrown, I was able to install your package by aliasing the readable-stream package and that solves my problem!

npm i readable-stream@npm:vite-compatible-readable-stream

[y-nk]

hello @mcollina ,

it's not clear from the thread of this issue if this issue was resolved or not. as i understood there was no blocker from here ; do you need some support to end this issue?

thanks

[gluck]

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.

[y-nk]

@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)

[thekevinbrown]

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.

[brunodb3]

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

[lemoustachiste]

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

[btakita]

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.

[WesSouza]

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);
    },
  },
];