search

sindresorhus / get-stream

Get a stream as a string, Buffer, ArrayBuffer or array
MIT License
341 stars 33 forks source link

Use flowing mode #6

Closed novemberborn closed 8 years ago

novemberborn commented 8 years ago

Needed to fix https://github.com/sindresorhus/ava/issues/590. Might be safest to treat this as a breaking change.

Also fixed the data listener closing over ret and (presumably) leaking the stream content for as long as the stream itself is in memory.

Updated Travis and engines to support Node 0.10.

Fixes #5.

novemberborn commented 8 years ago

Aw crap Node 0.10 doesn't have Promise of course. Weirdly in AVA we register it in the cli test which causes get-stream to work.

@sindresorhus we could require a polyfill to be installed, and then use one in the test if necessary? Alternatively we could export a factory method so a get-stream instance can be created with a custom promise module, like throat does.

sindresorhus commented 8 years ago

Is there any benefit of doing this other than Node 0.10 support? @julien-f mentioned something in #5.

novemberborn commented 8 years ago

Flowing mode seems better suited to this module. And means you can hand off a partially read stream to get-stream and get the remaining data.

sindresorhus commented 8 years ago

Might be safest to treat this as a breaking change.

Why exactly is it a breaking change?

novemberborn commented 8 years ago

Why exactly is it a breaking change?

It changes how the stream behaves. Before get-stream would read one chunk at a time, at its own leisure. Now it'll try and push data into get-stream as fast as possible. I don't think it'd emit readable events anymore. If users called other methods on the stream, rather than just passing it off to get-stream, the behavior of the stream may change with this update.

I have to admit that's largely theoretical though, and I don't 100% understand the subtleties in Node streams.

sindresorhus commented 8 years ago

And means you can hand off a partially read stream to get-stream and get the remaining data.

Sorry for my noobness when it comes to streams, but why can't you do this with a pull stream as it currently is? I thought pull streams could do everything "legacy" flow streams can, just with the added benefit of backpressure support.

sindresorhus commented 8 years ago

Before get-stream would read one chunk at a time, at its own leisure. Now it'll try and push data into get-stream as fast as possible.

But the end result is the same and so is the speed as with both ways we're operating at maximum speed. Either pulling data (pull-stream) or getting data pushed (push-stream) at it, it's still JS and single-threaded and can only work as fast as JS can concatenate strings/buffers.

sindresorhus commented 8 years ago

https://strongloop.com/strongblog/whats-new-io-js-beta-streams3/

sindresorhus commented 8 years ago

// @jamestalmage @vdemedes You guys have any stream experience?

vadimdemedes commented 8 years ago

I've got some, but not at such a deep level. This PR is pretty straightforward, implementing a usual method of reading a stream, so I don't think it brings any issues.

novemberborn commented 8 years ago

I thought pull streams could do everything "legacy" flow streams can, just with the added benefit of backpressure support.

Yes, though it's trickier to add a data listener when the stream is not in flowing mode (and without causing it to switch to flowing mode). This bit is what seems broken on Node 0.10 and is needed for the watcher test in AVA.

Using flowing mode here avoids those problems. There's on real benefit in using pull streams here, as far as I can tell.

sindresorhus commented 8 years ago

I'm still not 100% convinced about this. If it brings some actual benefits having it in flowing mode, I'm all for it, but it seems it's just to get Node 0.10 support, which honestly then would be better as a separate module called get-stream-legacy or something.

There's no real benefit in using pull streams here, as far as I can tell.

But there's no real benefit of using push streams on newer Node.js versions either, as far as I can tell.

novemberborn commented 8 years ago

But there's no real benefit of using push streams on newer Node.js versions either, as far as I can tell.

Flowing mode allows for multiple consumers. Pull streams kinda do if you're careful.

Then there's this quote from the stream docs on the data event:

If you just want to get all the data out of the stream as fast as possible, this is the best way to do so.

I'm all for it, but it seems it's just to get Node 0.10 support, which honestly then would be better as a separate module called get-stream-legacy or something.

Sure, I can write something else for our AVA test scenario. Just let me know. I can still do a new PR with the memory leak fix if you want.

jamestalmage commented 8 years ago

:+1: There is no reason not to do it this way. The nature of get-stream is such that we don't care about back pressure. Flowing mode still has full support AFAIK. Unless someone knows of plans to deprecate it, I say merge

sindresorhus commented 8 years ago

Alright, thanks guys. I'm convinced now.