Skip to content

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 a team, addaleax, mafintosh and mcollina May 31, 2018 10:14
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 <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
@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.

6 participants