search

nodejs / readable-stream

Node-core streams for userland
https://nodejs.org/api/stream.html
Other
1.03k stars 227 forks source link

You're fooling people's static analysis with "circular dependency" #280

Closed grantila closed 7 years ago

grantila commented 7 years ago

While checking for circular dependencies (using the webpack plugin circular-dependency-plugin) I found that out of my 1156 dependent packages only this and url-parse got complaints about circular dependencies.

ERROR in Circular dependency detected:
node_modules/readable-stream/lib/_stream_duplex.js -> node_modules/readable-stream/lib/_stream_readable.js -> node_modules/readable-stream/lib/_stream_duplex.js

ERROR in Circular dependency detected:
node_modules/readable-stream/lib/_stream_readable.js -> node_modules/readable-stream/lib/_stream_duplex.js -> node_modules/readable-stream/lib/_stream_readable.js

ERROR in Circular dependency detected:
node_modules/readable-stream/lib/_stream_writable.js -> node_modules/readable-stream/lib/_stream_duplex.js -> node_modules/readable-stream/lib/_stream_writable.js

Yes, this can be manually excluded with exclude: /node_modules\/readable-stream/ for all developers ending up with this problem, but the responsibility kind of bounces back here.

Even if it's fine at run-time, could you please break this circular dependency? You're fooling people's static analysis.

This issue is the same as https://github.com/unshiftio/url-parse/issues/49, and because circular dependencies doesn't seem very common, fixing the few packages out there with this problem would be good for the community.

calvinmetcalf commented 7 years ago

First off thanks for opening this, but I think we're coming at this from the wrong angle, you have a plugin that detects circular dependencies because sometimes they are bad, the plugin can't detect which circular dependencies are ok (at run time or avoiding module.exports) and which aren't (not at run time or using module.exports). I'd argue the best course of action here is to try to fix the plugin to b able to avoid false positives instead of trying to get libraries to fix non existent problems because your tooling has bugs.

calvinmetcalf commented 7 years ago

I'm going to close this with a note from the circular-dependency-plugin readme

circular dependencies are often a necessity in complex software, the presence of a circular dependency doesn't always imply a bug, but in the case where the you believe a bug exists, this module may help find it.

grantila commented 7 years ago

Please describe how to fix the static analyzer, if you know of an algorithm that can detect problematic and non-problematic circular dependencies statically. I think you underestimate the complexity.

Well, now you know that more and more people will stumble upon this, and as I wrote in the react-bootstrap issue, they will rightfully question your choice to implement a file-level circular dependency.

This can be fixed, as all file-level circular dependencies can be. Just so you know.

yoshuawuyts commented 7 years ago

@grantila Hey, if you have a problem with an implementation go ahead and fix it yourself. People staffing this repo are unpaid volunteers, demanding we put in work to solve a bug on your side is, well, rude. I hope you can be more understanding going forward. Thanks!

On Tue, Apr 25, 2017, 19:05 Gustaf Räntilä notifications@github.com wrote:

Please describe how to fix the static analyzer, if you know of an algorithm that can detect problematic and non-problematic circular dependencies statically. I think you underestimate the complexity.

Well, now you know that more and more people will stumble upon this, and as I wrote in the react-bootstrap issue, they will rightfully question your choice to implement a file-level circular dependency.

This can be fixed, as all file-level circular dependencies can be. Just so you know.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nodejs/readable-stream/issues/280#issuecomment-297097749, or mute the thread https://github.com/notifications/unsubscribe-auth/ACWlerVWHPpDIGt1NZFSuOMKGgl-pbToks5rzifXgaJpZM4NHlj7 .

grantila commented 7 years ago

@yoshuawuyts I totally understand, I do the same with my repos. So don't get me wrong, I'm just saying this design (file-level circular dependencies) is always going to be somewhere between ok and bad, and never necessary. Hopefully this helps you in the future. And it makes sense for you to know that this library causes "issues" when people run static analyzers, hence the GitHub "issue" ;)

I don't demand anything and I am understanding if you don't prioritize or care about this. I just want you to know about it! I've locally resolved this, so it doesn't bother me. Well unless disabling this package may cause my static analyzer to not find other circular dependencies that I might have wanted to know about.

calvinmetcalf commented 7 years ago

created aackerman/circular-dependency-plugin#16, more generally @grantila you're making an assumption that circular dependencies are by definition wrong and should be avoided and that is a position that we have failed to be convinced of.