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

doc: clarify Readable paused/flowing!==object mode #22619

Closed
wants to merge 1 commit into from
Closed

doc: clarify Readable paused/flowing!==object mode #22619

wants to merge 1 commit into from

Conversation

cxw42
Copy link
Contributor

@cxw42 cxw42 commented Aug 31, 2018

  • Clarify that a Readable stream's reading mode (paused vs. flowing)
    is independent of its object mode (object vs. non-object). I am
    relatively new to Node streams, and was briefly confused while
    reading the docs by the two uses of the word "mode".

  • Copyediting: add missing apostrophes; minor grammatical changes

Checklist

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Aug 31, 2018
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this PR is 💯!

* `readable.readableFlowing = true`
* `readable.readableFlowing == null`
* `readable.readableFlowing == false`
* `readable.readableFlowing == true`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe switching to === makes sense, given the “flexible” nature of normal JS equality comparisons?

Copy link
Contributor Author

@cxw42 cxw42 Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your quick review and positive feedback! This change is fine with me --- since I don't know the internals, I didn't want to assume. I will update momentarily. Edit Changed in 432fe1225b1ac8d4812f572357c9dfc0e4582291

@vsemozhetbyt
Copy link
Contributor

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 31, 2018
Copy link
Member

@lundibundi lundibundi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 👍

P.s. seems like source-links in docs are starting to pay off).


When `readable.readableFlowing` is `null`, no mechanism for consuming the
streams data is provided so the stream will not generate its data. While in this
state, attaching a listener for the `'data'` event, calling the
stream's data is provided. Therefore, the stream will not generate data.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: two spaces before 'Therefore'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - will fix along with the change in response to your comment below.

@@ -661,19 +664,19 @@ pass.unpipe(writable);

pass.on('data', (chunk) => { console.log(chunk.toString()); });
pass.write('ok'); // will not emit 'data'
pass.resume(); // must be called to make 'data' being emitted
pass.resume(); // must be called to make 'data' be emitted
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, wdyt of: 'must be called to make stream emit data'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do. However, I have a concern about implying determinism. What about "now the stream can emit 'data' events"? Correct me if I am wrong, but I think calling resume() does not mean that a data event will necessarily be emitted, depending on the state of the stream.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be emitted after resume if there is data in the stream and we have just added it there via the above write call. Therefore it should be fine with being deterministic here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased, and both of your changes applied, in 0bb51b0. Thanks!

- Clarify that a `Readable` stream's reading mode (paused vs. flowing)
  is independent of its object mode (object vs. non-object).  I am
  relatively new to Node streams, and was briefly confused while
  reading the docs by the two uses of the word "mode".

- Copyediting: add missing apostrophes; minor grammatical changes
@lundibundi
Copy link
Member

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

vsemozhetbyt pushed a commit that referenced this pull request Sep 3, 2018
- Clarify that a `Readable` stream's reading mode (paused vs. flowing)
  is independent of its object mode (object vs. non-object).  I am
  relatively new to Node streams, and was briefly confused while
  reading the docs by the two uses of the word "mode".

- Copyediting: add missing apostrophes; minor grammatical changes

PR-URL: #22619
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@vsemozhetbyt
Copy link
Contributor

Landed in 4f70ecc (with some exra spaces deleted).
Thank you!

targos pushed a commit that referenced this pull request Sep 3, 2018
- Clarify that a `Readable` stream's reading mode (paused vs. flowing)
  is independent of its object mode (object vs. non-object).  I am
  relatively new to Node streams, and was briefly confused while
  reading the docs by the two uses of the word "mode".

- Copyediting: add missing apostrophes; minor grammatical changes

PR-URL: #22619
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@cxw42 cxw42 deleted the patch-1 branch September 3, 2018 16:19
targos pushed a commit that referenced this pull request Sep 6, 2018
- Clarify that a `Readable` stream's reading mode (paused vs. flowing)
  is independent of its object mode (object vs. non-object).  I am
  relatively new to Node streams, and was briefly confused while
  reading the docs by the two uses of the word "mode".

- Copyediting: add missing apostrophes; minor grammatical changes

PR-URL: #22619
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants