-
Notifications
You must be signed in to change notification settings - Fork 29.5k
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
stream: propagate AbortSignal reason #55473
stream: propagate AbortSignal reason #55473
Conversation
Review requested:
|
Thanks for the contribution! Unfortunately, your first commit doesn't follow the guidelines set in place. Could you amend it? In this case, |
8feb3f6
to
ed0d894
Compare
There you go @RedYetiDev! 😉 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #55473 +/- ##
==========================================
- Coverage 88.42% 88.40% -0.02%
==========================================
Files 653 653
Lines 187498 187498
Branches 36100 36108 +8
==========================================
- Hits 165791 165762 -29
- Misses 14957 14965 +8
- Partials 6750 6771 +21
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Can you add it to the docs as well?
@mcollina I did not add it to the docs initially as it's the continuation of #41008, where no docs were updated, and it's now pretty much the expected behavior. However, if you think it's still worth it to add it to the docs, I'll do! |
Landed in 4a00f9a |
PR-URL: #55473 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
This PR propagates the reason of the
AbortSignal
passed to thepipeline
function (node:stream/promises
) to thecause
of the thrownAbortError
.The vast majority of
AbortError
s created by the standard library already propagates the reason, so this should bring more consistency.