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

streams: make the pipeline callback mandatory #21054

Closed
wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

Right now when not adding a callback to the pipeline it could cause
an uncaught exception if there is an error. Instead, just make the
callback mandatory as mostly done in all other Node.js callback APIs
so users explicitly have to decide what to do in such situations.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Right now when not adding a callback to the pipeline it could cause
an uncaught exception if there is an error. Instead, just make the
callback mandatory as mostly done in all other Node.js callback APIs
so users explicitly have to decide what to do in such situations.
@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 31, 2018
@BridgeAR BridgeAR requested review from mcollina, mafintosh, addaleax and a team May 31, 2018 10:14
@BridgeAR
Copy link
Member Author

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.

definitely LGTM

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 31, 2018
@BridgeAR
Copy link
Member Author

I forgot to update the docs. I just pushed a new commit.

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

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

@addaleax addaleax added the stream Issues and PRs related to the stream subsystem. label Jun 7, 2018
@addaleax
Copy link
Member

addaleax commented Jun 7, 2018

Landed in 32c51f1 (with s/streams/stream/ in the commit message)

@addaleax addaleax closed this Jun 7, 2018
addaleax pushed a commit that referenced this pull request Jun 7, 2018
Right now when not adding a callback to the pipeline it could cause
an uncaught exception if there is an error. Instead, just make the
callback mandatory as mostly done in all other Node.js callback APIs
so users explicitly have to decide what to do in such situations.

PR-URL: #21054
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR deleted the require-pipeline-callback branch January 20, 2020 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants