FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: unify writableErrored and readableErrored by ronag · Pull Request #40799 · nodejs/node · GitHub

/ node Public

stream: unify writableErrored and readableErrored - #40799

Closed
ronag wants to merge 2 commits into
nodejs:masterfrom
nxtedition:errored
ConversationCommits (2)ChecksFiles changed
Closed

stream: unify writableErrored and readableErrored#40799
ronag wants to merge 2 commits into
nodejs:masterfrom
nxtedition:errored

Conversation

ronag commented Nov 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Both of these should always refer to the same error, hence there is no reason to separate them.

Refs: #40696

stream: unify writableErrored and readableErrored
21f94b7 
Both of these should always refer to the same error, hence
there is no reason to separate them.
ronag added the stream Issues and PRs related to the stream subsystem. label Nov 13, 2021
ronag requested a review from mcollina November 13, 2021 12:43

ronag commented Nov 13, 2021

Copy link
Copy Markdown
Member Author

@nodejs/streams

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 13, 2021
mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 13, 2021
addaleax reviewed Nov 13, 2021
View reviewed changes
Comment thread doc/api/stream.md
called in order to fully uncork the stream.

##### `writable.writableErrored`
##### `writable.errored`

addaleax Nov 13, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

needs changes: entries?

ronag Nov 13, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

These props have never made it to any release... is that really necessary?

jasnell reviewed Nov 13, 2021
View reviewed changes
Comment thread lib/internal/streams/readable.js
},

readableErrored: {
errored: {

jasnell Nov 13, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Do the readableErrored and writableErrored need to go through a deprecation cycle first? Those should likely be runtime deprecated aliases, yes?

ronag Nov 13, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Not really. They haven’t been in any release.

lpinca removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 13, 2021

lpinca commented Nov 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

I've remove the semver-major label as per #40799 (comment).

ronag added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 13, 2021
mcollina approved these changes Nov 13, 2021
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

nodejs-github-bot commented Nov 15, 2021

Copy link
Copy Markdown
Collaborator

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

This was referenced Nov 16, 2021
Open
Open
Open

nodejs-github-bot commented Nov 18, 2021

Copy link
Copy Markdown
Collaborator

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

mcollina approved these changes Nov 18, 2021
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

nodejs-github-bot commented Nov 18, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Nov 18, 2021

Copy link
Copy Markdown
Collaborator

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

ronag commented Nov 18, 2021

Copy link
Copy Markdown
Member Author

Landed in 340b770

ronag closed this Nov 18, 2021
ronag added a commit that referenced this pull request Nov 18, 2021
stream: unify writableErrored and readableErrored
340b770 
Both of these should always refer to the same error, hence
there is no reason to separate them.

PR-URL: #40799
Refs: #40696
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Nov 19, 2021
Open
Open
Open
Open
Open
Open
BethGriggs mentioned this pull request Apr 9, 2022
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax left review comments
mcollina mcollina approved these changes
jasnell jasnell approved these changes
lpinca lpinca approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL