stream: only end reading on null, not undefined

[reem]

The Stream documentation for .push
explicitly states multiple times that null is a special cased value
that indicates the end of a stream. It is confusing and undocumented
that undefined also ends the stream, even though in object mode
there is a distinct and important difference.

The docs for Object-Mode also explicitly mention null as the only
special cased value, making no mention of undefined.

[indutny]

LGTM

[indutny]

Landed in 7fa4a96, thank you!

[UltCombo]

Although I conceptually agree with this reasoning, is such a small change really worth merging and breaking back-compat?

For a real use case example, I can no longer use gulp-watch as it depends on node-stream-array which relies on the old behavior of accepting undefined to denote the end of the readable stream.

I believe there should be more packages out there which rely on this behavior.

Wouldn't it be more proper to add a legacy note to the docs and keep back-compat?

//cc @indutny

[bjouhier]

Why would undefined be valid in an object stream, and not null? It should be the opposite.

An object stream should be able to carry any value that is serializable in JSON. Then it should allow null and use undefined as end of stream marker.

IMO either you allow both as end of stream marker, or only undefined.

[rlidwka]

An object stream should be able to carry any value that is serializable in JSON. Then it should allow null and use undefined as end of stream marker.

Yep, it makes more sense actually.

[UltCombo]

Accepting only undefined will break a heck lot of packages. So +1 to accepting both undefined and null again.

[brycebaril]

Ok, just spent some time debugging some modules which broke because of this change. I also am not a fan.

[brycebaril]

This also diverges from http://npm.im/readable-stream which does not have this patch -- @rvagg or @chrisdickinson any thoughts here?

[rvagg]

Need @chrisdickinson to weigh in on this but messing with the external API for streams is a huge risk at this stage and shouldn't be done lightly. There's a ton of code in npm that depend on minor details of the actual implementation and changes like this may be best seen as documentation problems rather than implementation problems.

That said, if you want stability, use readable-stream@1.0.x, this is a 1.1.x detail for streams3.

[chrisdickinson]

I am filled with unease by the thought that streams2 streams will still roam the earth despite streams3 being available. If this change is going to encourage that outcome, I'd almost prefer to back it out. On the other hand, we may be moving to a world where undefined and null are not "out of alphabet" for streams in the future, and this change is a first step in that direction (if unintentionally).

I think I have to come down on the side of backing this change out. If we're going to get to a world where undefined and null are in-alphabet, we have to do so in a way that is messaged in big bold letters to everyone it affects, with ample time to make the necessary changes.

@brycebaril I am curious, for future reference: how did the bug manifest?

[brycebaril]

@chrisdickinson in http://npm.im/stream-spigot (a Readable stream helper) there is an Array helper that creates streams from arrays e.g. spigot.array(["these", "will", "be", "stream", "chunks"])

It is (roughly) implemented as this.push(array.shift()) and thus the undefined was signalling the end.

The specific manifestation was in an objectMode stream causing an undefined property access error.

Edit: tests smoked this out immediately when using versions with this patch.