doc: add note to readable stream async iterator docs #19331

prog1dev · 2018-03-13T21:03:24Z

While I was working on #18904 I noticed that async iterator in readable streams iterates by the chunks of the size equal to stream highWaterMark value when reading a file. So I think it worth to mention in docs because at least for me it seemed confusing at first.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

stream, doc

Trott · 2018-03-13T21:11:42Z

doc/api/stream.md

@@ -1197,7 +1197,9 @@ print(fs.createReadStream('file')).catch(console.log);

 If the loop terminates with a `break` or a `throw`, the stream will be
 destroyed. In other terms, iterating over a stream will consume the stream
-fully.
+fully. 
+Note that by default file iteration would be by the chunks of the size equal


Thanks for the pull request! If this is a new paragraph, please put a blank line between it and the previous line. If it is not, please remove the newline. Thanks!

Not sure what you mean by that. There is a 80 chars per line restriction:

Running Markdown linter on docs... doc/api/stream.md 1201:153 warning Line must be at most 80 characters maximum-line-length remark-lint

Trott · 2018-03-13T21:13:16Z

doc/api/stream.md

-fully.
+fully. 
+Note that by default file iteration would be by the chunks of the size equal
+to [`highWaterMark`][] value(default is 16kb).


There appears to be no link for highWaterMark so this does not render as a link.

Please put a space between value and (.

Trott · 2018-03-13T21:30:34Z

Left some comments, but I'm happy to make the changes myself to your branch if you want. But I'm not exactly sure what the text means. Is it saying that the size of chunks read from the stream will be 16Kb by default? Or is something else 16Kb by default? I'm not sure what "file iteration" is. Does that mean reading the contents of a file?

prog1dev · 2018-03-13T21:41:01Z

Oh if its not clear from the first glance then I guess my note should be refactored 😄
Here is a piece of code similar to one in docs example. I just changed stream highWaterMark:

async function print(readable) {
  readable.setEncoding('utf8');
  readable._readableState.highWaterMark = 1;
  let data = '';
  let iterations = 0;
  for await (const k of readable) {
    data += k;
    iterations += 1;
  }
  console.log(data);
  console.log(iterations);
}

print(fs.createReadStream('file')).catch(console.log);

So without readable._readableState.highWaterMark = 1; line there is only one iteration but with it for me its 65 iterations. Here is a contents of my 'file':

first qweqweqwe
second line
third one
qwe fourth line
end of qwew

And I was wrong about the default value. Its 64 kb as fs.createReadStream doc says

prog1dev · 2018-03-13T23:03:47Z

I changed the wording
Hope its better now

BridgeAR · 2018-03-14T09:38:36Z

doc/api/stream.md

@@ -1197,7 +1197,10 @@ print(fs.createReadStream('file')).catch(console.log);

 If the loop terminates with a `break` or a `throw`, the stream will be
 destroyed. In other terms, iterating over a stream will consume the stream
-fully.
+fully. 
+Note that by default, async reading of the contents of the file will be


Nit: it should be obvious that this is a async operation, so I would not mention that again.

I still find this a bit confusing. For example, "of the file"...there's no guarantee the stream is a file, or am I wrong about that?

How about this?

By default, the stream will be read in chunks of size equal to the highWaterMark option.

(Maybe omit the mention of the default value from fs.createReadStream() since the default value is 16kb for some other ways of creating streams.)

Is "by default" even necessary in the sentence? Is there an option to change it? If so, maybe it should be mentioned? "If fooOption is not set, then by default, the stream will be..."

For example, "of the file"...there's no guarantee the stream is a file, or am I wrong about that?

Its just that I tested with fs.createReadStream() and the same example in docs but I guess this behaviour is the same in other cases

After thinking this though I think you are right in all of your suggestions. But note became too general so I would also add a test-case example:

The stream will be read in chunks of size equal to the highWaterMark option.
So in code example above it will only be one iteration if 'file' has less
then 64kb of data. Unless custom highWaterMark value is specified in
[fs.createReadStream()][] constructor.

Looks good?
If its too much hassle I can just left general part:

The stream will be read in chunks of size equal to the highWaterMark option.

Maybe this?

The stream will be read in chunks of size equal to the `highWaterMark` option. In the code example above, data will be in a single chunk if the file has less then 64kb of data because no `highWaterMark` option is provided to [`fs.createReadStream()`][].

Seems good 👍
thank you, updated in 3aa49be
its hard to write good docs 😄

Thanks for your patience. The PR-review process for docs can seem tedious and endless sometimes.

prog1dev · 2018-03-17T15:20:30Z

ping @BridgeAR @Trott @jasnell

Trott · 2018-03-17T16:03:35Z

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/265/

prog1dev · 2018-03-18T20:49:38Z

tests are green
I guess its okay to merge this?

PR-URL: nodejs#19331 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

Trott · 2018-03-19T02:01:31Z

Landed in 45c86e3

prog1dev · 2018-03-19T09:02:23Z

Yay! \o/

nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Mar 13, 2018

Trott reviewed Mar 13, 2018

View reviewed changes

prog1dev force-pushed the doc/add_note_to_stream_async_iterator_docs branch 2 times, most recently from 1c6d7ad to c221d8e Compare March 13, 2018 22:58

doc: add note to readable stream async iterator docs

d124874

prog1dev force-pushed the doc/add_note_to_stream_async_iterator_docs branch from c221d8e to 6e53943 Compare March 13, 2018 23:03

doc: rephrase the note to make it clearer

042cf85

prog1dev force-pushed the doc/add_note_to_stream_async_iterator_docs branch from 6e53943 to 042cf85 Compare March 13, 2018 23:07

BridgeAR approved these changes Mar 14, 2018

View reviewed changes

jasnell approved these changes Mar 14, 2018

View reviewed changes

doc: generalize the note after rewiew suggestions

3aa49be

Update stream.md

3088fb2

Trott pushed a commit to Trott/io.js that referenced this pull request Mar 19, 2018

doc: add note to readable stream async iterator

45c86e3

PR-URL: nodejs#19331 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

Trott closed this Mar 19, 2018

MylesBorins added dont-land-on-v6.x labels Mar 20, 2018

prog1dev deleted the doc/add_note_to_stream_async_iterator_docs branch March 29, 2018 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: add note to readable stream async iterator docs #19331

doc: add note to readable stream async iterator docs #19331

prog1dev commented Mar 13, 2018 •

edited

Loading

Trott Mar 13, 2018

prog1dev Mar 13, 2018

Trott Mar 13, 2018

Trott Mar 13, 2018

prog1dev Mar 13, 2018

Trott commented Mar 13, 2018

prog1dev commented Mar 13, 2018 •

edited

Loading

prog1dev commented Mar 13, 2018

BridgeAR Mar 14, 2018

Trott Mar 14, 2018 •

edited

Loading

Trott Mar 14, 2018

Trott Mar 14, 2018

prog1dev Mar 15, 2018

Trott Mar 15, 2018 •

edited

Loading

prog1dev Mar 16, 2018 •

edited

Loading

Trott Mar 16, 2018

prog1dev commented Mar 17, 2018

Trott commented Mar 17, 2018

prog1dev commented Mar 18, 2018

Trott commented Mar 19, 2018

prog1dev commented Mar 19, 2018

doc: add note to readable stream async iterator docs #19331

doc: add note to readable stream async iterator docs #19331

Conversation

prog1dev commented Mar 13, 2018 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Mar 13, 2018

prog1dev commented Mar 13, 2018 • edited Loading

prog1dev commented Mar 13, 2018

Choose a reason for hiding this comment

Trott Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

prog1dev Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prog1dev commented Mar 17, 2018

Trott commented Mar 17, 2018

prog1dev commented Mar 18, 2018

Trott commented Mar 19, 2018

prog1dev commented Mar 19, 2018

prog1dev commented Mar 13, 2018 •

edited

Loading

prog1dev commented Mar 13, 2018 •

edited

Loading

Trott Mar 14, 2018 •

edited

Loading

Trott Mar 15, 2018 •

edited

Loading

prog1dev Mar 16, 2018 •

edited

Loading