Vote on the approach for https://github.com/nodejs/node/issues/445

[mcollina]

In nodejs/node#445, the streams WG was tasked to remove the access of _writableState and _readableState across the various areas of core.

Three approaches are possible:

a. leave _writableState and _readableState as is, and just document them. A variation is to remap them without the _, but keep them in the same object. This would cause to expose a lot of other properties we do not need.

b. expose the part of the state as properties/getters on the stream object themselves, with the disadvantage that the streams would get some more properties.

c. allocate a new object for writableState and readableState and use getters there to link them to the _writableState and _readableState. This would force the creation of 2 more objects for each Duplex, and the allocation of a stream is already slow. Plus, it creates more references to clean up by the GC.

For the Streams WG, the best choice is option b), followed by option a) (do nothing).

In nodejs/node#445 and nodejs/node#12857 @mscdex expressed a formal -1 on the approach, preferring option c).

We should vote on the following:

1. Can we add readableLength and writableLength to Readable and Writable respectively stream: remove {writeableState/readableState}.length node#12857?
2. Can we add other properties like the above to Readable and Writable do remove the need to access _readableState and _writableState Cleanup _writableState and _readableState access across codebase node#445?

[jasnell]

+1 for option B. +1 for adding readableLength and writableLength and +1 for adding other public properties as necessary.

[addaleax]

I am in favour of option B, for both items. (Is there any reason to have different stances on readableLength/writableLength vs the other properties?)

[mcollina]

@addaleax no, not really, one is a concrete example of the other.

[mscdex]

Option A is a little confusing. It says to leave the properties as-is and document them. However that option is then later referred to as "do nothing." Documenting the properties (further) would be more than "do nothing."

I would not be opposed to a "do nothing" option, meaning no further documentation of the properties.

To reiterate, I am against Option B because it creates a lot of unnecessary noise on objects and having the properties properly corralled into separate objects (whether via the current properties or new, separate properties for that matter) keeps it cleaner. I think we should strive to be less intrusive with userland objects.

[Trott]

I'm abstaining on this one. Happy to defer to more informed and invested folks on this.

[evanlucas]

If a is for documenting the current properties, then I vote for a.

[MylesBorins]

I'm abstaining myself. I am willing to put in time to get up to speed on this problem if my vote is necessary.

[cjihrig]

I'm leaning toward Option B, but have concerns about colliding with properties that userland modules may have created.

[mcollina]

@mscdex I was not clear. I do not think "do nothing" is a feasible approach. Some properties have to be documented, otherwise it's not possible to explain how streams works. IMHO whatever we use within core should be documented, as it is very likely the most advanced usecase anyway.

Is there any reason to have different stances on readableLength/writableLength vs the other properties?)

@addaleax It is just one example of such addition. There are a bunch of other PRs around, which I did not keep up-to-date because it take some grinding and this has been going on for months.

I'm leaning toward Option B, but have concerns about colliding with properties that userland modules may have created.

@cjihrig we can land the changes as semver-major, or use prefixes like readableLengh or readableHighWaterMark, those are very long (and maybe ugly) function names, but they should work.

[Fishrock123]

As noted in nodejs/node#445 (comment):

Option a seems unfortunate but more favorable. We should probably not do nodejs/node#12857 then.

Or, at least I thought a was more favorable. If the streams WG thinks b is ok I agree it would be better.

[mcollina]

We are ok with both b) "add properties" and a) do-nothing at code-level (and maybe documenting things as needed). This was a request that was born from within core, not from the streams WG.

I'm happy with whatever strategy we could find consensus on, because currently we are stuck and there are a bunch of PRs open that cannot land or be closed.

If the consensus is do-nothing-and-do-not document, then we are grand with that as well. But currently you cannot explain how streams work without looking at _readableState and _writableState (which is why those are mentioned in the docs). My tendency would be to document the state as needed, i.e. not all properties needs to be document.

[mscdex]

Has anyone tested using proxies as a solution with very recent versions of V8 (e.g. at least 6.2)? I read there have been some improvements in ES2015 Proxy performance. If the performance is good there, we might just wait until that version of V8 lands and then add proxies, which allows us to avoid documenting the private properties (at least further).

[refack]

@bmeck what's your experiment with proxies indicate?

[bmeck]

@refack my code isn't as hot as streams, but the perf seems ok-ish. Roughly dictionary mode object but not easy to tell w/o a hotter benchmark test.

[mcollina]

@mscdex how would you use proxies in this situation?

[TimothyGu]

I have the same question as @mcollina. And if I were eligible to vote I would choose b). This isn't really about keeping the object cleaner, but making sure it would be possible for streams to evolve.

As far as Proxy performance goes, I've tested out Proxies on the ToT V8 for jsdom, which admittedly uses Proxies more extensively than most would. The results in a nutshell are, Proxies have become a lot faster recently, but are still slow compare to normal objects. But I guess that is to be expected.

[mhdawson]

+1 for option B.

[targos]

I abstain.

[Trott]

There are 22 members of the TSC. 3 have abstained so far. As of now, that means an option needs 10 votes.

I see 5 votes for option B (@jasnell @addaleax @Fishrock123 @cjihrig @mhdawson) and 1 vote for option A (@evanlucas).

So this has a ways to go before resolution. By all means, if you're on @nodejs/tsc, invested, and undecided: keep discussing.

But if you're definitely abstaining, or if you have an opinion and haven't registered it (ping @mcollina!), please drop a note here.

[ofrobots]

I am leaning towards option B.

[bnoordhuis]

Abstain. No strong or defensible position.

[joyeecheung]

Abstain

[ChALkeR]

I abstain until noted otherwise (probably won't happen). ;-)
I am not in favor of doing nothing on this, though.

[Fishrock123]

To clarify my position: Option B since the streams WG prefers it.

[Trott]

Update:

• Abstentions: 7 (@Trott @MylesBorins @targos @bnoordhuis @joyeecheung @ChALkeR @thefourtheye)
• "Yes" votes required to pass an option: 8
• Option A: 1 (@evanlucas)
• Option B: 7 (@jasnell @addaleax @cjihrig @mhdawson @ofrobots @Fishrock123 @mcollina)
• Opposed to both: 1 (@mscdex)

Not yet voted or abstained: @fhinkel @indutny @joshgav @rvagg @shigeki @trevnorris

Note that you can vote for or against both options (like @mscdex did).

[mcollina]

I vote for option B.

[mcollina]

Hey folks, what's the status on this? cc @fhinkel @indutny @joshgav @rvagg @shigeki @thefourtheye @trevnorris can you please express an opinion or abstain?

[mhdawson]

Added review and agenda tags in case its not resolved by the next meeting.

[jasnell]

I would give this a firm deadline and consider anyone that hasn't voted by then to be abstentions.

[thefourtheye]

Abstaining.

[mcollina]

Let's call this done on tomorrow TSC meeting.

[Trott]

One more vote (or two more explicit abstentions) from any of @fhinkel @indutny @rvagg @trevnorris could seal this. Someone, please?

[rvagg]

+1 for option B from me, sorry for the delay

[Trott]

It's official! Option B! I'm going to close this and remove the tsc-* labels. Feel free to re-open, comment, re-add the label, whatever if you think it's warranted.

[michaelnisi]

Confining scope to the relevant, most used, properties of _readableState/_writableState and exposing those separately sounds like the best approach. Exposing internal objects, even just as getters, still leads to dependent user land modules, constraining internal flexibility.

Rolling this in gradually might be feasible:

• _readableState/_writableState restricted to getters
• Add separate properties while deprecating getters (incorporating feedback)
• Remove _readableState/_writableState getters