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: Update backpressuring-in-streams.md by introducing pipeline since v10.0.0 #1758

Merged
3 commits merged into from Aug 4, 2018
Merged

doc: Update backpressuring-in-streams.md by introducing pipeline since v10.0.0 #1758

3 commits merged into from Aug 4, 2018

Conversation

ghost
Copy link

@ghost ghost commented Aug 2, 2018

Ref: #1668

PS:Considering the method is introduced since v10.0.0, and we can still use chains of pipe() together, so I still keep the original codes and explainations. But just add for new introductions of pipeline().

@ghost ghost requested review from jalafel and fhemberger August 2, 2018 04:20
@targos
Copy link
Member

targos commented Aug 2, 2018

/cc @mcollina @mafintosh

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. I’ll soon be releasing readable-stream@3 that enables pipeline in Node 6+.
We should be updating this accordingly.

@sonicdoe
Copy link
Contributor

sonicdoe commented Aug 2, 2018

If stream.pipeline() fully replaces pump, I’d suggest rewording the section so that it is introduced first and to mention that pump is only necessary for Node.js v8 or earlier.

What do you think? If you’d like, I can push a small example.

Copy link

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time to improve this

[`pump`][]. This is a module method to pipe between streams forwarding errors
and properly cleaning up and provide a callback when the pipeline is complete.

You can use it like this following:
Copy link

Choose a reason for hiding this comment

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

to stay consistent with other parts of the document, lets rephrase this:

Here is an example of using pipeline:

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

}
);
```
or if you wanna `async` with `await`, you can use it wrapped by [`promisify`][]:
Copy link

Choose a reason for hiding this comment

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

Let's replace "wanna" since it is not formal english

You can call [promisify][] on pipeline to use it with async/await:

// Either of the following ways you can call the `run()`:

// Way 1: You can use this to catch exceptions in async mode.
run().catch((err) => console.log('Pipeline failed', err));
Copy link

Choose a reason for hiding this comment

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

I feel this is redundant, maybe just having the following code snippet is enough:

async function run() {
  try {
    await pipeline(
       fs.createReadStream('The.Matrix.1080p.mkv'),
       zlib.createGzip(),
       fs.createWriteStream('The.Matrix.1080p.mkv.gz'),
    );
  } catch(err) {
    console.error('Pipeline failed', err);
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@Bamieh
Copy link

Bamieh commented Aug 2, 2018

Closes #1668


// Use the pipeline API to easily pipe a series of streams
// together and get notified when the pipeline is fully done.
// A pipeline to gzip a potentially huge tar file efficiently:
Copy link
Contributor

Choose a reason for hiding this comment

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

tar file -> video file?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@ghost
Copy link
Author

ghost commented Aug 3, 2018

@vsemozhetbyt, @sonicdoe & @Bamieh
All have been fixed according to your ideas :)

@sonicdoe:The pump is a 3-rd party tool with an example of a link, and if you click that it will guide you there (https://github.com/mafintosh/pump). Since there's already a link direct to GitHub with an example there. And I'm not sure whether we can allow a 3-rd party example directly in Nodejs? If you are glad to do this, maybe you can submit another PR after mine.

@Bamieh:Maybe when this submit is merged, you can close your issue, or I'll help to close.

@ghost ghost merged commit 493715f into nodejs:master Aug 4, 2018
@ghost ghost deleted the IntroducePipeline branch August 4, 2018 08:16
@ghost
Copy link
Author

ghost commented Aug 4, 2018

@mcollina:I've merged this, and waiting for your changes onto my submitted one. Then I'll call other translators to make a translation on what we've changed.

@mcollina
Copy link
Member

mcollina commented Aug 4, 2018

I think you can go ahead and start translating this change. It might take me a bit more time, and I do not want to stop any activity.

@ghost
Copy link
Author

ghost commented Aug 5, 2018

OK. Thanks! @mcollina

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants