★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/18375Nouvelle recherche | Portail wanayoo
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: make it more exact #18375

Closed
wants to merge 3 commits into from
Closed

Conversation

MoonBall
Copy link
Member

@MoonBall MoonBall commented Jan 25, 2018

I made a comment on every change.

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

handlers, and removing all pipe destinations by calling the
[`stream.unpipe()`][] method.
* If there are pipe destinations, by removing all pipe destinations by
calling the [`stream.unpipe()`][] method.
Copy link
Member Author

@MoonBall MoonBall Jan 25, 2018

Choose a reason for hiding this comment

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

only calling unpipe() will make stream to paused mode. see the following example.

const { Readable, Writable } = require('stream');

const r = new Readable();
const w = new Writable();

r._read = () => {};
w._write = () => {};

r.pipe(w);
r.on('data', () => {});

r.unpipe();
console.log(r._readableState.flowing); // the output is false

Copy link
Member

@jasnell jasnell Jan 27, 2018

Choose a reason for hiding this comment

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

quick grammar nit on this: by removing all pipe destinations by ... the sentence should be something like:

Multiple pipe destinations may be removed by calling the [`stream.unpipe()`][] method.

Copy link
Member Author

@MoonBall MoonBall Jan 27, 2018

Choose a reason for hiding this comment

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

fixed.

buffered. Once the `callback` is invoked, the stream will emit a [`'drain'`][]
event. If a stream implementation is capable of processing multiple chunks of
data at once, the `writable._writev()` method should be implemented.
buffered. When the `callback` is invoked, the stream maybe will emit a
Copy link
Member Author

@MoonBall MoonBall Jan 25, 2018

Choose a reason for hiding this comment

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

Emitting a 'drain' event need to meet more conditions. See the source code of writable stream.

Copy link
Member

@Trott Trott Jan 25, 2018

Choose a reason for hiding this comment

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

maybe will -> might

Copy link
Member Author

@MoonBall MoonBall Jan 26, 2018

Choose a reason for hiding this comment

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

fixed.

@maclover7
Copy link
Member

@maclover7 maclover7 commented Jan 26, 2018

BridgeAR added a commit to BridgeAR/node that referenced this issue Feb 1, 2018
PR-URL: nodejs#18375
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 1, 2018

Landed in 3ca7935

@BridgeAR BridgeAR closed this Feb 1, 2018
MylesBorins added a commit that referenced this issue Feb 20, 2018
PR-URL: #18375
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins added a commit that referenced this issue Feb 21, 2018
PR-URL: #18375
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins added a commit that referenced this issue Feb 21, 2018
PR-URL: #18375
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
MylesBorins added a commit that referenced this issue Mar 20, 2018
PR-URL: #18375
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins added a commit that referenced this issue Mar 28, 2018
PR-URL: #18375
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins added a commit that referenced this issue Mar 30, 2018
PR-URL: #18375
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
MayaLekova added a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18375
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants