stream: make readable & writable computed #31197

ronag · 2020-01-05T17:43:57Z

This makes readable and writable automatically computed based
on the stream state.

Effectivly deprecating/discouraging manual management of this.

Makes the properties more consistent and easier to reason about.

Refs: #29377

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

ronag · 2020-01-05T21:34:36Z

ping @mcollina

ronag · 2020-01-05T22:19:44Z

semver-major

nodejs-github-bot · 2020-01-05T23:17:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28222/

lib/_http_client.js

lib/internal/http2/core.js

ronag · 2020-01-31T19:23:50Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2254/ (queued)

ronag · 2020-02-01T11:54:06Z

Needs TSC review. @mcollina @jasnell @addaleax

doc/api/stream.md

lib/_stream_readable.js

lib/_stream_writable.js

Co-Authored-By: Anna Henningsen <github@addaleax.net>

nodejs-github-bot · 2020-02-01T12:23:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28849/

lpinca

Seems good to me.

test/parallel/test-stream-readable-readable.js

nodejs-github-bot · 2020-02-05T14:46:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28938/

nodejs-github-bot · 2020-02-05T15:55:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28942/

addressed

addaleax · 2020-02-07T11:44:05Z

This needs another @nodejs/tsc review

mcollina

lgtm - I would prefer to hear from @mafintosh before landing, but we can go ahead.

mafintosh · 2020-02-07T16:55:34Z

Lgtm from me also

Trott · 2020-02-07T18:36:56Z

doc/api/stream.md

@@ -495,7 +495,8 @@ added: v11.4.0

 * {boolean}

-Is `true` if it is safe to call [`writable.write()`][stream-write].
+Is `true` if it is safe to call [`writable.write()`][stream-write], which means
+the stream has not been destroyed, errored or ended.


Suggested change

the stream has not been destroyed, errored or ended.

the stream has not been destroyed, errored, or ended.

^Non-blocking, in case that wasn't obvious. (FWIW, our Style Guide says to use serial commas.)

Trott · 2020-02-07T18:39:45Z

CITGM results look good.

Trott

LGTM with the streams subject-matter-expert approval that it has.

ronag · 2020-02-08T22:26:10Z

Landed in e559842

This makes readable and writable automatically computed based on the stream state. Effectivly deprecating/discouraging manual management of this. Makes the properties more consistent and easier to reason about. Fixes: #29377 PR-URL: #31197 Refs: #29377 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

A remainder from a previous refactoring. Refs: nodejs#31197

A remainder from a previous refactoring. Refs: #31197 PR-URL: #31805 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jan 5, 2020

ronag force-pushed the stream-writable-computed branch from 5151533 to 3833c20 Compare January 5, 2020 17:44

This comment has been minimized.

Sign in to view

ronag force-pushed the stream-writable-computed branch 2 times, most recently from c55b3ca to e753f27 Compare January 5, 2020 20:12

ronag changed the title ~~stream: Writable.writable computed~~ stream: readable & writable computed Jan 5, 2020

ronag force-pushed the stream-writable-computed branch 6 times, most recently from e1bcd2a to c968c49 Compare January 5, 2020 21:06

lundibundi added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 5, 2020

This comment has been minimized.

Sign in to view

ronag force-pushed the stream-writable-computed branch from c2e76c0 to 46368c6 Compare January 6, 2020 00:16

ronag commented Jan 6, 2020

View reviewed changes

lib/_http_client.js Outdated Show resolved Hide resolved

ronag commented Jan 6, 2020

View reviewed changes

lib/internal/http2/core.js Outdated Show resolved Hide resolved

ronag force-pushed the stream-writable-computed branch 10 times, most recently from 80ea96c to 52f36d0 Compare January 6, 2020 12:43

ronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Jan 31, 2020

addaleax approved these changes Feb 1, 2020

View reviewed changes

doc/api/stream.md Outdated Show resolved Hide resolved

doc/api/stream.md Outdated Show resolved Hide resolved

lib/_stream_readable.js Show resolved Hide resolved

lib/_stream_writable.js Show resolved Hide resolved

Apply suggestions from code review

1c5112c

Co-Authored-By: Anna Henningsen <github@addaleax.net>

ronag requested a review from lpinca February 1, 2020 12:24

lpinca approved these changes Feb 1, 2020

View reviewed changes

test/parallel/test-stream-readable-readable.js Outdated Show resolved Hide resolved

ronag added 2 commits February 1, 2020 22:23

fixup: remove redundant mustCall

e71ceea

fixup: explicit cast to Boolean

6160cdb

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 7, 2020

mcollina approved these changes Feb 7, 2020

View reviewed changes

Trott reviewed Feb 7, 2020

View reviewed changes

Trott approved these changes Feb 7, 2020

View reviewed changes

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 8, 2020

ronag closed this Feb 8, 2020

ronag added a commit to nxtedition/node that referenced this pull request Feb 15, 2020

stream: avoid writing to writable

8dd4e55

A remainder from a previous refactoring. Refs: nodejs#31197

ronag mentioned this pull request Feb 16, 2020

stream: avoid setting writable state #31805

Closed

4 tasks

BethGriggs mentioned this pull request Mar 10, 2020

2020-04-21, Version 14.0.0 (Current) #32181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: make readable & writable computed #31197

stream: make readable & writable computed #31197

ronag commented Jan 5, 2020 •

edited

Loading

This comment has been minimized.

ronag commented Jan 5, 2020

ronag commented Jan 5, 2020

nodejs-github-bot commented Jan 5, 2020

This comment has been minimized.

ronag commented Jan 31, 2020

ronag commented Feb 1, 2020 •

edited

Loading

nodejs-github-bot commented Feb 1, 2020

lpinca left a comment

nodejs-github-bot commented Feb 5, 2020

nodejs-github-bot commented Feb 5, 2020

addaleax commented Feb 7, 2020

mcollina left a comment

mafintosh commented Feb 7, 2020

Trott Feb 7, 2020

Trott Feb 7, 2020

Trott commented Feb 7, 2020

Trott left a comment •

edited

Loading

ronag commented Feb 8, 2020

	the stream has not been destroyed, errored or ended.
	the stream has not been destroyed, errored, or ended.

stream: make readable & writable computed #31197

stream: make readable & writable computed #31197

Conversation

ronag commented Jan 5, 2020 • edited Loading

Checklist

This comment has been minimized.

ronag commented Jan 5, 2020

ronag commented Jan 5, 2020

nodejs-github-bot commented Jan 5, 2020

This comment has been minimized.

ronag commented Jan 31, 2020

ronag commented Feb 1, 2020 • edited Loading

nodejs-github-bot commented Feb 1, 2020

lpinca left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 5, 2020

nodejs-github-bot commented Feb 5, 2020

addaleax commented Feb 7, 2020

mcollina left a comment

Choose a reason for hiding this comment

mafintosh commented Feb 7, 2020

Trott Feb 7, 2020

Choose a reason for hiding this comment

Trott Feb 7, 2020

Choose a reason for hiding this comment

Trott commented Feb 7, 2020

Trott left a comment • edited Loading

Choose a reason for hiding this comment

ronag commented Feb 8, 2020

ronag commented Jan 5, 2020 •

edited

Loading

ronag commented Feb 1, 2020 •

edited

Loading

Trott left a comment •

edited

Loading