search

rvagg / through2

Tiny wrapper around Node streams2 Transform to avoid explicit subclassing noise
MIT License
1.9k stars 106 forks source link

require readable-stream@3 #99

Closed stephenplusplus closed 5 years ago

stephenplusplus commented 5 years ago

This change would require readable-stream@3, instead of being okay with 2 or 3. The current workaround for a project is to depend on both through2@3 and readable-stream@3, but it seems easier for through2 to choose one version of readable-stream@3. Am I overlooking something with that idea?

Thank you for the library and considering!

(RE: https://github.com/googleapis/nodejs-storage/pull/799#issuecomment-519678551)

stephenplusplus commented 5 years ago

@rvagg sorry to ping, but just wanted to see if you think this is a likely addition here or not so much. I'm happy to take on further effort if required.

rvagg commented 5 years ago

Soooooo .. this is a little bit complicated but I don't think that replacing through2 with a custom stream is entirely unreasonable. I've been tending toward this as have others who helped maintain this module. With the introduction of the simplified constructor, it makes it harder to justify using through2 for most things: https://nodejs.org/api/stream.html#stream_simplified_construction

Unfortunately that issue you've linked raises one of the problems - this is only OK if you can guarantee you're on a newer version of Node or you use readable-stream directly. The custom stream in that PR is waaay over complicated, but it seems to be necessitated by the desire to avoid readable-stream as a dependency--if I were maintaining a library that needed strict support across all versions of Node but needed a custom stream then pulling in readable-stream is entirely reasonable and a good thing. Or just accept readable-stream and use it directly or through2 along with readable-stream@3 so it gets selected.

On the logic in that thread:

In order to support asyncIterator through2 needs to use readable-sream@3, however, since through2 is OK with ether one (link), among requirements of other modules through2 ends up "deduped" to readable-sream@2.

So readable-stream is being pulled in by something else already, yet there's a desire to not use readable-stream so remove through2? Are they going to remove whatever else is pulling in readable-stream to be consistent? And if they do that, then they end up with the ability to use through2 again because it'll get readable-stream@3 again! Seems a little circular to me. Just include readable-stream@3 in your package.json and there will be peace in the world.

If I pin to readable-stream@3 here then I'm going to get all sorts of complaints from others playing wac-a-mole with streams, that's why the 2||3 exists in the first place.

Happy to hear input from other through2 maintainers, but I don't think this is going to happen as things stand now.

stephenplusplus commented 5 years ago

@rvagg thank you very much for the thoughtful reply. We ended up pinning ^3 in the repos we needed it. I'm going to close the PR here-- feel free to re-open if this is something you want to follow through with. Thanks for your help!