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

Jump to bottom

lib: aggregate errors to avoid error swallowing #39985

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

lib: aggregate errors to avoid error swallowing #39985

wants to merge 4 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 3, 2021

No description provided.

@aduh95 aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 3, 2021
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Sep 3, 2021
VoltrexKeyva
VoltrexKeyva reviewed Sep 3, 2021
View changes
doc/api/net.md Outdated Show resolved Hide resolved
VoltrexKeyva
VoltrexKeyva reviewed Sep 3, 2021
View changes
doc/api/stream.md Outdated Show resolved Hide resolved
VoltrexKeyva
VoltrexKeyva reviewed Sep 3, 2021
View changes
doc/api/stream.md Outdated Show resolved Hide resolved
VoltrexKeyva
VoltrexKeyva reviewed Sep 3, 2021
View changes
doc/api/stream.md Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Sep 3, 2021
View changes
doc/api/http.md Outdated Show resolved Hide resolved
@mscdex
Copy link
Contributor

mscdex commented Sep 3, 2021

Missing tests?

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 4, 2021

Missing tests?

Most of this stuff is hard to test reliably (I didn't find a way to generate an error on closing a file descriptor), but I did add a test for http.ClientRequest change which is more easily testable.

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 15, 2021

Ping @nodejs/tsc for reviews.

mcollina
mcollina requested changes Sep 15, 2021
View changes
Copy link
Member

@mcollina mcollina left a comment

I would prefer to have a test for streams.

BridgeAR
BridgeAR reviewed Nov 4, 2021
View changes
Copy link
Member

@BridgeAR BridgeAR left a comment

I guess some of these are in fact exclusive errors. The aggregateTwoErrors() function already handles that case but it might make sense to use an assertion instead to guarantee that they are indeed exclusive.
Others might be direct causes of former errors and in those cases it's probably best to use the new JS error cause option.

Due to that I suggest to handle each case individually per PR or commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell

@BridgeAR BridgeAR

@VoltrexKeyva VoltrexKeyva

@mcollina mcollina

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@aduh95 @mscdex @mcollina @jasnell @BridgeAR @VoltrexKeyva @nodejs-github-bot