Skip to content
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

streams: update .readable/.writable to false #4083

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@mscdex
Copy link
Contributor

commented Dec 1, 2015

These properties were initially used to determine stream status back in node v0.8 and earlier. Since streams2 however, these properties were always true, which can be misleading for example if you are trying to immediately determine whether a Writable stream is still writable or not (to avoid a "write after
end" exception).

This is #1217 targeting the master branch and with a couple of minor changes to fix a lint error and to help address a concern about .on().

streams: update .readable/.writable to false
These properties were initially used to determine stream status
back in node v0.8 and earlier. Since streams2 however, these
properties were *always* true, which can be misleading for
example if you are trying to immediately determine whether
a Writable stream is still writable or not (to avoid a "write after
end" exception).
@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2015

CI is all green.

@chrisdickinson I've replaced the additional conditional used in .on() instead of removing it now. Does this LGTY?

@chrisdickinson

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2015

LGTM!

mscdex added a commit that referenced this pull request Dec 2, 2015

streams: update .readable/.writable to false
These properties were initially used to determine stream status
back in node v0.8 and earlier. Since streams2 however, these
properties were *always* true, which can be misleading for
example if you are trying to immediately determine whether
a Writable stream is still writable or not (to avoid a "write after
end" exception).

PR-URL: #4083
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Dec 2, 2015

Landed in cc0342a.

@mscdex mscdex closed this Dec 2, 2015

mscdex added a commit that referenced this pull request Dec 5, 2015

streams: update .readable/.writable to false
These properties were initially used to determine stream status
back in node v0.8 and earlier. Since streams2 however, these
properties were *always* true, which can be misleading for
example if you are trying to immediately determine whether
a Writable stream is still writable or not (to avoid a "write after
end" exception).

PR-URL: #4083
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>

@rvagg rvagg referenced this pull request Dec 17, 2015

Closed

Release 4.2.4 Planning #4321

@mscdex mscdex deleted the mscdex:fix-readable-writable branch Dec 17, 2015

@jasnell

This comment has been minimized.

Copy link
Member

commented Dec 17, 2015

@mscdex ... how would you rate the semver'iness of this change? Should this go into v4?

@jasnell

This comment has been minimized.

Copy link
Member

commented Dec 17, 2015

Marking this as lts-watch for v4 but please do not land in v4.x-staging until confirmed

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2015

@jasnell I would say no for v4 as it's more like a semver-major since it changes property values of existing properties (for readables it sets .readable = false sooner and for writables it actually sets .writable = false now).

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Dec 29, 2015

adding semver-major tag and removing LTS watch. Please feel free to update if I was incorrect in doing so

edit: if we do add it back to LTS we need to add the tag back to #4141 as well

@jasnell jasnell referenced this pull request Mar 17, 2016

Closed

Planning for v6 #5766

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

streams: update .readable/.writable to false
These properties were initially used to determine stream status
back in node v0.8 and earlier. Since streams2 however, these
properties were *always* true, which can be misleading for
example if you are trying to immediately determine whether
a Writable stream is still writable or not (to avoid a "write after
end" exception).

PR-URL: nodejs#4083
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.