Node: [stream] Make _readableState and _writableState part of the public API

[mcollina]

Following up from https://github.com/nodejs/node/pull/7077#issuecomment-231497169, and other discussion.

One of the problem that has lead to breaking userland stream modules is related to the fact that they rely on functionality available in _readableState and _writableState that is not documented, and that is the only possible way for solving some issues, like knowing if a stream is in objectMode: true, clearing up the leftover write callbacks, and many other functionalities.

I propose we document, standardize and test a piece of _readableState  and _writableState.

I think we should have been able to spot https://github.com/mafintosh/duplexify/issues/6 before releasing, sigh. @calvinmetcalf @TheAlphaNerd are we setting readable-stream in passthrough mode for citgm run?

I am opening this discussion here rather than in readable-stream because I would like all possible feedback. As @mikeal said, "So, what are we gonna do about streams?" https://github.com/nodejs/summit/issues/16#issuecomment-227552773.

@nodejs/streams we might want probably put this at in our next meeting.

cc @ronkorving @mafintosh

[vsemozhetbyt]

Are the '_readableStream' and '_writableStream' typos in the title?

[mcollina]

@vsemozhetbyt good catch!

[mscdex]

Related: https://github.com/nodejs/node/issues/6799

[mscdex]

I personally think that having a single, non-underscore-prefixed property (to minimize impact) to house all state would be ideal (e.g. stream.streamState with stream.streamState._readable, etc. as necessary).

[mafintosh]

Adding any properties by now will just make things more complicated in userland though as you'd have yet another thing to test for.

I'm 👍 on just making _readableState and _writableState part of the public readonly api - this is how they are used right now anyway.

[mscdex]

Well technically it'd be trading two existing properties for one ;-)

I just don't like setting a precedent by documenting an underscore-prefixed property. It's already bad enough that _readableState is already publicly "documented" and its use is being encouraged....

[mcollina]

@mscdex Technically a single property is not correct, as you can have a Transform that is in objectMode only on one side.

[vkurchatkin]

_readableState and _writableState should remain private. If some functionality is missing, it should be added to stream instances instead

[mafintosh]

adding the properties directly on the stream instances will likely break some modules as inheritance from streams is encouraged. it'll also make it complicated for module authors to determine if a property was set by the stream impl or the userland subclass as you dont know which version of streams an instance was created with

[vkurchatkin]

adding the properties directly on the stream instances will likely break some module

We have 2 options:

• find names that doesn't break any modules
• use static functions instead

[mcollina]

I'm not convinced on new properties, because it won't have an impact short-term. There are modules/applications that depends on these properties, because there aren't ways of solving certain issues using public APIs in a backward-compatible way.

[vkurchatkin]

Documenting properties, prefixed with underscore essentially destroys convention and will eventually lead to a situation where we have to document all private properties.

So, a that very least, we need to remove underscores.

[mcollina]

@vkurchatkin I agree. However the problem is that _modules_ uses the _-prefixed properties in the wild. At this point in time, it's _impossible_ for us to know what should we should semver about or not.
So, this solution helps in the long-term but not in the short-term.

I'm also a solution I could be happy with. My point is that we need to something to avoid this issue.

[lance]

Note this was also discussed here https://github.com/nodejs/node/issues/6799

[Trott]

Although conversation appears stalled, this hasn't reached a conclusion one way or the other, has it?

[mcollina]

There is no consensus on this one and all the relative issues. I can put to together a report for @nodejs/ctc, and we might want to pick a direction. I do not think we can reach consensus otherwise.

[caub]

I don't understand the few people who thumbed down

also maybe just simpler getters stream.readableState would return _readableState

[mcollina]

This can be closed, we are pursuing #445 and adding accessors for the properties that __needs__ access to _readableState  and _writableState. Sorry for the long wait, this could have been closed earlier.