-
Notifications
You must be signed in to change notification settings - Fork 28.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[stream] Make _readableState and _writableState part of the public API #7629
Comments
Are the '_readableStream' and '_writableStream' typos in the title? |
@vsemozhetbyt good catch! |
Related: #6799 |
I personally think that having a single, non-underscore-prefixed property (to minimize impact) to house all state would be ideal (e.g. |
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. |
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 |
@mscdex Technically a single property is not correct, as you can have a |
|
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 |
We have 2 options:
|
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. |
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. |
@vkurchatkin I agree. However the problem is that modules uses the I'm also a solution I could be happy with. My point is that we need to something to avoid this issue. |
Note this was also discussed here #6799 |
The MSI install scope was set to the WiX default, which is per-user. However, with UAC, it could not be installed by a standard user because InstallPrivileges is elevated by default, hence the install scope should be set to per-machine. Furthermore, the default install path is a per-machine location and setting the system path requires administrator privileges. By changing the InstallScope to perMachine, Start Menu shortcuts are placed in ProgramData and not the installing user's AppData folder, making the shortcuts available to other users. This also fixes the installation when AppData is a network folder. The custom action is necessary to allow upgrades. Since a per-machine MSI cannot upgrade an application installed per-user, the custom action checks if there is going to be an upgrade to a previous version installed per-user and sets the installation as per-user to allow upgrading. Hence, the advantages of installing per-machine will only apply in fresh installations. Fixes nodejs#5849 Fixes nodejs#7629 PR-URL: nodejs/node-v0.x-archive#25640 Reviewed-By: Alexis Campailla <alexis@janeasystems.com> Reviewed-By: Bert Belder <bertbelder@gmail.com>
Although conversation appears stalled, this hasn't reached a conclusion one way or the other, has it? |
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. |
I don't understand the few people who thumbed down also maybe just simpler getters |
This can be closed, we are pursuing #445 and adding accessors for the properties that needs access to |
Following up from #7077 (comment), 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 inobjectMode: 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 mafintosh/duplexify#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?" openjs-foundation/summit#16 (comment).
@nodejs/streams we might want probably put this at in our next meeting.
cc @ronkorving @mafintosh
The text was updated successfully, but these errors were encountered: