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: supplementary explanation for 'readable' event #16932

Closed
wants to merge 0 commits into from

Conversation

Aaaaaaaty
Copy link

Checklist

  • documentation is changed or added

Affected core subsystem(s)

supplementary explanation for 'readable' event

@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 Nov 10, 2017
@benjamingr
Copy link
Member

Hey, thanks for the contribution!

I don't think the docs are clearer after that change than before it. Sorry.

@Aaaaaaaty
Copy link
Author

@benjamingr I just find that sometimes new data pushed to the queue will not trigger the readable event =.= . I think we should point out it

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.

Thanks for contributing! I left a little comment, as I think the paragraph is still missing some information.

@@ -774,7 +774,7 @@ end
*Note*: In general, the `readable.pipe()` and `'data'` event mechanisms are
easier to understand than the `'readable'` event.
However, handling `'readable'` might result in increased throughput.

in the Meanwhile, new data will not trigger `'readable'` event everytime.
Copy link
Member

Choose a reason for hiding this comment

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

“In” should be capitalized. ‘readable’ is emitted if ‘read()’ returned null the last time it was called (from memory, please verify).

@fhinkel
Copy link
Member

fhinkel commented Nov 11, 2017

@Aaaaaaaty Thanks so much for your first commit 🎉. Do you want to take a stab at the changes @mcollina suggested?

@Aaaaaaaty
Copy link
Author

Aaaaaaaty commented Nov 11, 2017

@fhinkel @mcollina hi I just noticed that directly execute _read () to push the data except using read(), it will not trigger readable. So I think maybe it is better to point out it. Although few people use it in that way =.=. I will update my pr and maybe you can tell me a good way to explain this condition. Thanks all~

@@ -774,7 +774,7 @@ end
*Note*: In general, the `readable.pipe()` and `'data'` event mechanisms are
easier to understand than the `'readable'` event.
However, handling `'readable'` might result in increased throughput.

In the Meanwhile, new data will not trigger `'readable'` event everytime.
Copy link
Member

Choose a reason for hiding this comment

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

  • meanwhile rather than Meanwhile, although I'm not sure if In the meanwhile is necessary or clear. The word meanwhile suggests something else is happening. It's not clear what is happening. Maybe use "While" instead? Like "While the foo event is queued but has not yet been emitted, new data will not..."?

Smaller nits:

  • trigger a `'readable'` event rather than trigger 'readable' event`
  • every time rather than everytime.
  • blank line after this line

The nits can be handled by someone landing the PR but if you're going in to edit some more anyway, maybe take care of them then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants