stream: add isFlowing method

[vkurchatkin]

Add isFlowing method to detect if readable stream is in a flowing mode. !isPaused() is not enough since there is a third "initial" state.

see #445

/cc @chrisdickinson

[sonewman]

This PR should prob be submitted to https://github.com/iojs/readable-stream also

[chrisdickinson]

@sonewman readable-stream currently has a script for slurping up changes from upstream repos. Until we've got that flow reversed, this repo is still the correct place for streams PRs.

[chrisdickinson]

Hm, I am a little conflicted here. Should isFlowing() be hung off of _readableState instead (is it an API private to io.js, or do consumers need to know about it?)

[chrisdickinson]

(isPaused is sort of the same way -- at one point I thought it was a really good idea, but I'm not convinced it needs to be public facing now)

[vkurchatkin]

Well we have isPaused, it's only logical to have isFlowing. It can be useful for consumers too:

function doSomethingWithAStream(stream) {
  stream.on('data', ...); // can accidentally switch into flowing mode
}

function doSomethingElseWithAStream(stream) {
  var flowing = stream.isFlowing();
  stream.on('data', ...); // can accidentally switch into flowing mode
  if (!flowing) stream.pause();
}

Should isFlowing() be hung off of _readableState instead (is it an API private to io.js)

If it's public then more things can be done in userland. That's the point of this issue. If doing something requires knowledge about internals, that means that some things a just impossible to implement outside of the core.

[sonewman]

@chrisdickinson I agree it should be on the _readableState, it's interesting because in my experiments with streams internals, i thought about adding prototype methods to the various _*States.
I am not completely convinced that it needs to be a function though, i would probably added a getter isFlowing instead. (TIMTOWTDI)

[chrisdickinson]

@vkurchatkin What I meant to say was: "I added isPaused publicly, but now I'm kind of regretting it." One problem with growing the public API is that there's no guarantee that a stream you've been handed actually supports the full API! For example: it could be a streams 1 stream (via through), or an old version of readable-stream, or (now) a node stream. Another problem is that it exposes the inner workings of the state machine to the end user – which prevents us from changing those inner workings.

Re: the code example: that API is attainable at present through the somewhat-roundabout EventEmitter.prototype.on.call(stream, 'data', function) approach. In the future we should plan on deprecating the implicit flowing of streams via "data" listeners.

[vkurchatkin]

In the future we should plan on deprecating the implicit flowing of streams via "data" listeners.

Definitely +1 on that. But I can only imagine how painful it would be.

[sonewman]

@chrisdickinson would that mean that you would need to explicitly called .resume() if you wanted to start receiving data events. I guess it enforces the user to take control of that stream, rather than the opposite (being forced to call .pause() before allowing any handles to be added) mind you if you allowed implementers to determine their own streams inner strategy then it could be explicitly determined by the stream.

It would mean the implementer would need to document the use of the module more throughly in order for consumers to known what to expect.

[chrisdickinson]

@sonewman I'm not sure how I feel about making all kinds of flowing explicit like that – that bears more thought than I've given it. I mean that we'd nix .on('data') and only support .pipe and .resume as methods for "starting" a stream. This would let us turn the tri-value flowing property into a proper boolean value and make the specialization of .on a little less surprising.

[vkurchatkin]

This would let us turn the tri-value flowing property into a proper boolean value

this looks like a totally separate issue. I can see no real purpose of that, but it definitely makes things difficult (like we have to flowing = null;). It will be a breaking change though

[sonewman]

@vkurchatkin this is currently how the readable stream works.

The null value is the initial state _stream_readable.js#L83.

If you add a data handle and this value is null then it starts flowing mode _stream_readable.js#L707-L709.

If you call pause it will set _readableState.flowing = false _stream_readable.js#L766-L770.

This means that if you call .pause() on a stream and then add an on data listener,
then add a listener to the readable event, when data is .push()ed into the readable then the data listener acts as a passive listener on the stream.

This is where the tri-value comes from.

@chrisdickinson is suggesting simplifying that API to make the control more explicit and remove the tri-value state.

[vkurchatkin]

@sonewman what I mean is, we can remove "tri-value state" right away. It only affects the result of isPaused, otherwise null and false values are the same. But it's a breaking change a would require major version bump.

[sonewman]

@vkurchatkin it would definitely be a breaking change, all streams using the on data event would break and they would be incompatible with the currently version of streams, because when you add an on data listener it sets the stream to flowing if it is true or null.

[rvagg]

we have "semver-minor" and "semver-major" tags now, can someone decide what this is (if any) and tag this please?

[vkurchatkin]

@rvagg this should be semver-minor, but I'm just going to close it