Node: Stream writable/readable properties are undocumented as of streams2

[strugee]

• Version: 10
• Platform: N/A
• Subsystem: doc

stream.Writable#writable is no longer documented, but according to https://stackoverflow.com/a/23094413/1198896 it exists and probably _should_ be documented. Presumably the same is true for stream.Readable#readable?

[killagu]

+1

[thatshailesh]

https://nodejs.org/docs/v0.9.4/api/stream.html#stream_class_stream_writable
It was there before @Trott would like to know why it was removed? can we add again?

[Trott]

@nodejs/streams

[mcollina]

I think these should be documented. They are used in the wild.

@strugee would you like to send a PR?

[thatshailesh]

I am also willing to help with PR
@strugee let me know if you're not sending :)

[strugee]

Hey, sorry for the delay! I looked into this briefly and realized I'd have to dig through the source to make sure I had the right implementation details, and I haven't had time to do that yet. @thatshailesh given the situation, if you want to take this then by all means go ahead! Otherwise I can do this when I find some time :)

[thatshailesh]

Ok Sure, I'll send it thanks :)

[thatshailesh]

@strugee Hey I just found this
Writable - https://nodejs.org/dist/latest-v10.x/docs/api/stream.html#stream_constructor_new_stream_writable_options
Readable - https://nodejs.org/dist/latest-v10.x/docs/api/stream.html#stream_implementing_a_readable_stream

Both are there in the doc, hope it resolves the issue :)

[felixrabe]

@thatshailesh - no, you linked to the uppercase Readable and Writable classes, whereas this issue is about the lowercase readable and writable properties which are still (last I checked) undocumented.

[strugee]

@felixrabe is correct. What we are looking for is something like https://nodejs.org/docs/v0.8.0/api/stream.html#stream_stream_readable, but note that that's for 0.8 streams and not streams2, which is why I said I'd have to dig into the implementation to make sure I wrote something correct.

[mcollina]

Those properties are sill there, and I suspect they still work as before. I think they are still there for compatibility reason. You might want to add unit tests for them if there are none.

[ronkorving]

So, I just wrote an issue and closed it as a duplicate. It is however not exactly a duplicate, but probably worth covering in the same breath as this issue.

net.Socket has a writable property. It does not initialize it in its constructor, so it depends on the Writable for it, which sets it to true in its constructor. After a successful connection, net.Socket sets it to true, which seems rather pointless since it was already true from construction time. Maybe this code is acceptable, if we decide that Writable owns it, and Writable stream implementations are always responsible for keeping that value correct.

[mcollina]

@ronkorving I think Readable and Writable should be responsibile to set and maintain those values. Would you like to send a PR in that regard?

[ronkorving]

@mcollina Can they though? net.Socket sets writable to true the moment connect() is called on it. I don't think there's a Writable hook there that could be depended upon to set that boolean instead. Got any suggestions?

[mcollina]

net.Socket can set it to false after calling the Writable constructor. The
machinery for setting it to false on destroy should be there. It can also
be part of the refactor myself and @mafintosh plan to do.
Il giorno mar 31 lug 2018 alle 21:36 Ron Korving [e-mail]
ha scritto:

@mcollina https://github.com/mcollina Can they though? net.Socket sets
writable to true the moment connect() is called on it. I don't think
there's a Writable hook there that could be depended upon to set that
boolean instead. Got any suggestions?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/issues/21431#issuecomment-409420134, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AADL4wmXtv5c4ZAoZilxrWgc8rvQrszHks5uMQY1gaJpZM4Uwo5r
.

[ronkorving]

@mcollina So then you're suggesting we move the entire property to net.Socket, right? Rather than Readable and Writable as I think you were suggesting in your previous comment? I don't mind if you guys pick it up of course 👍

[mcollina]

We are a bit strained atm, so if you want to send a PR it would be very welcomed.

IMHO we should have those in stream.Readable and stream.Writable and document them. However, net.Socket could flip the value on startup if we want to be backward compatible. Given that the change would likely be semver-major anyway, I'm actually thinking that we should _only_ have those properties in Readable  and Writable, and they should start both as true (because .read() and .write() will not error right after creation).

[ronkorving]

@mcollina I fully agree with that approach. I may make a PR, but am a bit strained myself.

[antsmartian]

This is fixed in : https://github.com/nodejs/node/pull/23933. Hence closing it. Please re-open if I'm incorrect.