-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: unify writableErrored and readableErrored #40799
Conversation
Both of these should always refer to the same error, hence there is no reason to separate them.
@nodejs/streams |
@@ -621,7 +621,7 @@ added: | |||
Number of times [`writable.uncork()`][stream-uncork] needs to be | |||
called in order to fully uncork the stream. | |||
|
|||
##### `writable.writableErrored` | |||
##### `writable.errored` |
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.
needs changes: entries?
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.
These props have never made it to any release... is that really necessary?
@@ -1239,7 +1239,7 @@ ObjectDefineProperties(Readable.prototype, { | |||
} | |||
}, | |||
|
|||
readableErrored: { | |||
errored: { |
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.
Do the readableErrored
and writableErrored
need to go through a deprecation cycle first? Those should likely be runtime deprecated aliases, yes?
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.
Not really. They haven’t been in any release.
I've remove the semver-major label as per #40799 (comment). |
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
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
Landed in 340b770 |
Both of these should always refer to the same error, hence there is no reason to separate them.
Refs: #40696