Repository navigation
doc,streams: internal "private" properties are publicly documented #6799
Description
Activity
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_readableStateso I'm not sure what we can reasonably do other than perhaps create a non-prefixedreadableStatealias.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)_metaproperty.@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...
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.
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_eventsfrom EventEmitter, plenty of them use_readableStateand_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 —_readableStateand_writableStatehave changed internally in the recent past, and are likely to continue to change in the near future.In other words,
_readableStateis 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.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.
Reacted by Lance Ball and Anna Henningsen@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.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_readableStateobject 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 (likereadable-streamof 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?
@lance Consider the outcome: users can't rely on
.readableState, because the stream they're dealing with may come from an older version ofreadable-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._readableStatefor the foreseeable future.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
_readableStateis 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_readableStatewill 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.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
_readableStatefrom the docs.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.
Some random points:
- tons of modules uses
_readableStateand_writableState. I use it to check what type of stream it is. _readableStateand_writableStateare 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).- 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
_readableStatetoreadableStateand start documenting (some) of the properties. People will keep using_readableStatefor the time being, and in 1 or 2 LTS we can get rid of_readableState.- tons of modules uses
ping @nodejs/streams
- added a commit that references this issue
on Apr 13, 2018 - added a commit that references this issue
on Apr 16, 2018 - added a commit that references this issue
on May 1, 2018
I just noticed that not only is
_readableStatepublicly 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_readableStateand 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