doc,streams: internal "private" properties are publicly documented

[mscdex]

• Version: v0.10.14+
• Platform: all
• Subsystem: doc, streams

I just noticed that not only is _readableState publicly documented, but its access is being encouraged in the documentation too. The text was added way back in db5776c. This doesn't feel right. It seems like we should be either not documenting these kinds of properties or we should be providing a non-underscored API if we really want that functionality to be public. I know that users will tap into _readableState and similar properties anyway for various reasons (and that in the past there were some attempts to fix those reasons), but publicly documenting them and encouraging their use is not right IMHO.

/cc @nodejs/streams

[jasnell]

very much +1... we can't be telling users that _-prefixed properties are private in some cases but go around recommending their use in others. A quick search tho shows a ton of hits for _readableState so I'm not sure what we can reasonably do other than perhaps create a non-prefixed readableState alias.

[chrisdickinson]

I'm -1 on removing the documentation for _readableState — at this point the property is thoroughly used in the ecosystem. Adding a non-underscored property won't eliminate that usage. For an example of a similar situation elsewhere, see also Django's (formerly private) _meta property.

[mscdex]

@chrisdickinson What is the harm in adding an alias like @jasnell suggested and documenting that instead? Sure it would take awhile for people to migrate, but at least new users perusing the documentation wouldn't be encouraged to use the underscored property anymore...

[lance]

Agree that adding a non-underscored alias and documenting it is the right way to go. Encouraging the use of anything with an underscore prefix is a mixed message. These properties are traditionally considered private, and exposing one might make it seem reasonable to dig through the node sources and find others, which is a bad idea, IMO.

[chrisdickinson]

I'm not sure that _ is a useful signal for "absolutely don't use this," but more of a "please be careful using this." Plenty of modules use _events from EventEmitter, plenty of them use _readableState and _writableState. We can't realistically remove these properties without causing a lot of breakage, or warn deprecation for them since so many modules depend (directly or indirectly) on those properties. Aliasing them doesn't make a lot of sense, either — _readableState and _writableState have changed internally in the recent past, and are likely to continue to change in the near future.

In other words, _readableState is a gray area — it's a semi-public API. We don't want to suggest that it's a blessed API by removing the _ since it's still likely to change, but we can't remove it either. I think the best case scenario is to document it, and note that it's potentially unstable and that users should take care. I am -1 on aliasing the object to a new property, and -1 on deprecating/removing the old property in the long run.

[mscdex]

I've never viewed underscore-prefixed properties as "semi-public." To me those have always been strictly considered private. Usage of underscore-prefixed properties is typically a sign that a public API needs to be added in order get at the appropriate data in a reliable way. Doing this would allow the underlying implementation to change without affecting users.

I'm still a hard -1 on documenting underscore-prefixed properties.

[lance]

@chrisdickinson I see your point about breakage, but when I first came to Node and saw what the ecosystem was doing with these _ variables I was dismayed. By convention, I have always understood _ to signal private ownership. Sure, it's just convention, and never actually stated anywhere, but it has never set well with me. Frankly, I think it's a sign of design flaw that these _ properties are exposed at all; there ways to make private data truly private - but that's a whole other ball of wax. IMO the right way forward is to properly expose functions that provide what's needed as @mscdex says, and deprecate/warn on undesirable usage.

[chrisdickinson]

I've never viewed underscore-prefixed properties as "semi-public." To me those have always been strictly considered private. Usage of underscore-prefixed properties is typically a sign that a public API needs to be added in order get at the appropriate data in a reliable way. Doing this would allow the underlying implementation to change without affecting users.

A well-thought-out API that gets at the information that _readableState & friends contain would be a much more acceptable approach than exposing the _readableState object un-prefixed. However, the problem is that trying to expand the public API surface area of streams is difficult — it runs into the same problem as promises, in that streams are passed between packages, and different implementations (like readable-stream of varying versions) exist. New surface area can't be relied upon to exist.

For what it's worth, our dependents have already paved this cowpath. I don't believe it's worthwhile trying to divert it because we disagree with how they paved it. Outside of our personal preferences, how does changing this property make the platform materially better for our users?

[chrisdickinson]

@lance Consider the outcome: users can't rely on .readableState, because the stream they're dealing with may come from an older version of readable-stream. This stream may not originate within their package, it might be handed to them from another package, transitively. Users will continue to have to use ._readableState for the foreseeable future.

[lance]

Outside of our personal preferences, how does changing this property make the platform materially better for our users?

Long term stability.

We don't want to suggest that it's a blessed API by removing the _ since it's still likely to change

So, if _readableState is likely to change, then users can't reliably depend on it long-term anyway. At some point their code is likely to break - transitive dependency or not. The fact that users depend on this property illustrates a gap in the API that should be addressed.

I can appreciate the difficulty of expanding the stream API surface area, and understand that changes like this don't happen overnight. But rather than just accepting what is there now, I believe that creating a path towards long-term stability is the better way to go. At some point in the future, perhaps, most modules will have migrated to .readableState() and deprecating/eliminating _readableState will not be so painful. But to just leave it as-is, that will never happen, and users will still be in this gray area - depending on a semi-private property that's likely to change.

[chrisdickinson]

Long term stability.

I don't follow how aliasing a property pointing at an unstable API aids the cause of long term stability.

I can appreciate the difficulty of expanding the stream API surface area, and understand that changes like this don't happen overnight. But rather than just accepting what is there now, I believe that creating a path towards long-term stability is the better way to go. At some point in the future, perhaps, most modules will have migrated to .readableState() and deprecating/eliminating _readableState will not be so painful. But to just leave it as-is, that will never happen, and users will still be in this gray area - depending on a semi-private property that's likely to change.

Fair enough. In that case, in lieu of exposing an unstable API as stable, we should define the needs that are not being served by the current public API and fill them in — not expose the existing, unstable API. Once we've introduced those changes in a release we can remove _readableState from the docs.

[lance]

Long term stability.
I don't follow how aliasing a property pointing at an unstable API aids the cause of long term stability.

OK. You got me there. I did champion the cause of a non-underscored alias in an earlier comment. Can I take that back?

we should define the needs that are not being served by the current public API and fill them in

Yes, I agree.

[mcollina]

Some random points:

1. tons of modules uses _readableState and _writableState. I use it to check what type of stream it is.
2. _readableState and _writableState are part of the supported API anyway, meaning that we cannot change them without expecting a lot of breakage (beware that through readable-stream the breakage will be massive).
3. there is the "bad habit" of breaking inside the internals in node as well.

we should define the needs that are not being served by the current public API and fill them in

I'm slightly against this. The problem of using a set of different properties to achieve that means that either we are introducing them as function properties (increasing code size significantly), or it is duplicating a lot of information (increasing memory size).

Code size is important for readable-stream and browsers.

I'm 👍 for aliasing _readableState to readableState and start documenting (some) of the properties. People will keep using _readableState for the time being, and in 1 or 2 LTS we can get rid of _readableState.

cc @mafintosh @calvinmetcalf

[jasnell]

ping @nodejs/streams

[mcollina]

This is being addressed in #12855.