Skip to content

stream: add pipeline() and finished() promises version - #33991

Closed
rickyes wants to merge 3 commits into
nodejs:masterfrom
rickyes:add-promises-stream
Closed

stream: add pipeline() and finished() promises version#33991
rickyes wants to merge 3 commits into
nodejs:masterfrom
rickyes:add-promises-stream

Conversation

@rickyes

@rickyes rickyes commented Jun 20, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes: #33582

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to Node.js builds or CI infrastructure. label Jun 20, 2020
@rickyes

rickyes commented Jun 20, 2020

Copy link
Copy Markdown
Contributor Author

If the feature is agreed, I will add the document.

@rickyes
rickyes force-pushed the add-promises-stream branch from d8f9628 to 8db937f Compare June 21, 2020 02:05
@rickyes rickyes changed the title stream: add pipeline() promises version stream: add pipeline() and finished() promises version Jun 21, 2020
@rickyes

rickyes commented Jun 21, 2020

Copy link
Copy Markdown
Contributor Author

/cc @ronag @mcollina @benjamingr

benjamingr
benjamingr reviewed Jun 21, 2020
View reviewed changes
Comment thread lib/stream.js Outdated
@benjamingr

benjamingr commented Jun 21, 2020

Copy link
Copy Markdown
Member

This generally looks fine but I have a habit to never approve streams PRs without checking the code out and thoroughly running it in cases I consider. I will be able to review this when I am back in the office after the summit (so early next week) but @ronag or @mcollina might have time before.

ronag
ronag reviewed Jun 21, 2020
View reviewed changes
Comment thread lib/internal/streams/promises.js Outdated
ronag
ronag reviewed Jun 21, 2020
View reviewed changes
Comment thread lib/internal/streams/promises.js Outdated
ronag
ronag reviewed Jun 21, 2020
View reviewed changes

@ronag ronag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm good with this.

@ronag ronag added stream Issues and PRs related to Node.js streams. and removed build Issues and PRs related to Node.js builds or CI infrastructure. labels Jun 21, 2020
@mcollina

mcollina commented Jun 21, 2020

Copy link
Copy Markdown
Member

I think this is in the right direction, having docs would be a nice step forward.

mcollina
mcollina reviewed Jun 21, 2020
View reviewed changes
Comment thread lib/internal/streams/promises.js Outdated
mcollina
mcollina reviewed Jun 21, 2020
View reviewed changes
Comment thread lib/stream.js Outdated
ronag
ronag reviewed Jun 21, 2020
View reviewed changes
Comment thread lib/internal/streams/promises.js Outdated
ronag
ronag reviewed Jun 21, 2020
View reviewed changes
Comment thread lib/stream.js Outdated
ronag
ronag reviewed Jun 21, 2020
View reviewed changes
Comment thread lib/stream.js Outdated
@rickyes
rickyes force-pushed the add-promises-stream branch from 0cba778 to 2b3e621 Compare June 21, 2020 15:12
@ronag ronag mentioned this pull request Jun 21, 2020
Closed
2 tasks done
jasnell
jasnell requested changes Jun 22, 2020
View reviewed changes
Comment thread lib/stream.js Outdated
@rickyes
rickyes force-pushed the add-promises-stream branch from c6aac8a to e5756db Compare June 23, 2020 13:19
@rickyes

rickyes commented Jun 23, 2020

Copy link
Copy Markdown
Contributor Author

I've made some changes. Please help review. @jasnell @mcollina @ronag

ronag
ronag reviewed Jun 23, 2020
View reviewed changes

@ronag ronag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm good with this if @jasnell & @mcollina are happy with the lazy loading.

jasnell
jasnell approved these changes Jun 23, 2020
View reviewed changes

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lgtm pending @mcollina signoff

mcollina
mcollina reviewed Jun 23, 2020
View reviewed changes
Comment thread lib/stream/promises.js Outdated
@rickyes

rickyes commented Jun 25, 2020

Copy link
Copy Markdown
Contributor Author

ping @mcollina

lundibundi
lundibundi approved these changes Jun 25, 2020
View reviewed changes
Comment thread lib/stream.js Outdated
Comment thread test/parallel/test-stream-promises.js Outdated
Comment thread test/parallel/test-stream-promises.js Outdated
Comment thread test/parallel/test-stream-promises.js Outdated
mcollina
mcollina approved these changes Jun 25, 2020
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@rickyes

rickyes commented Jun 26, 2020

Copy link
Copy Markdown
Contributor Author

@lundibundi done

trivikr
trivikr requested changes Jul 8, 2020
View reviewed changes
Comment thread doc/api/stream.md
@nodejs-github-bot

nodejs-github-bot commented Jul 8, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/32249/

@rickyes

rickyes commented Jul 8, 2020

Copy link
Copy Markdown
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/32249/

@ronag @trivikr The ci is passed.

@github-actions github-actions Bot mentioned this pull request Jul 9, 2020
Open
20 tasks
@ronag

ronag commented Jul 9, 2020

Copy link
Copy Markdown
Member

Landed in 527e214

ronag pushed a commit that referenced this pull request Jul 9, 2020
@rickyes @ronag
stream: add promises version to utility functions
527e214 
PR-URL: #33991
Fixes: #33582
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@ronag ronag closed this Jul 9, 2020
@rickyes
rickyes deleted the add-promises-stream branch July 9, 2020 09:33
This was referenced Jul 10, 2020
Open
Open
@BethGriggs BethGriggs mentioned this pull request Sep 1, 2020
Merged
@richardlau richardlau mentioned this pull request Oct 6, 2020
Merged
This was referenced Oct 24, 2020
Closed
Closed
@coderabbitai coderabbitai Bot mentioned this pull request Mar 24, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr left review comments
@mcollina mcollina mcollina approved these changes
@jasnell jasnell jasnell approved these changes
@ronag ronag ronag approved these changes
@trivikr trivikr trivikr approved these changes
+1 more reviewer
@lundibundi lundibundi lundibundi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to Node.js streams.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Promise-friendly stream.pipeline and stream.finished

9 participants

@rickyes @benjamingr @mcollina @targos @nodejs-github-bot @ronag @jasnell @lundibundi @trivikr