Skip to content

stream: readable error emitted - #29009

Closed
ronag wants to merge 2 commits into
nodejs:masterfrom
nxtedition:stream-readable-error-emitted
Closed

stream: readable error emitted#29009
ronag wants to merge 2 commits into
nodejs:masterfrom
nxtedition:stream-readable-error-emitted

Conversation

@ronag

@ronagronag commented Aug 6, 2019

Copy link
Copy Markdown
Member

Track whether error has been emitted on readable as well. This will allow some minor refactoring and optimisations on stream helpers.

Depends/Based on: #28709.

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

@nodejs-github-botnodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Aug 6, 2019
@ronag

ronag commented Aug 6, 2019

Copy link
Copy Markdown
MemberAuthor

Still need to consider what/how to test here.

@ronagronag mentioned this pull request Aug 6, 2019
7 tasks
@ronag
ronagforce-pushed the stream-readable-error-emitted branch from f86dba6 to 43093f5CompareAugust 6, 2019 13:58
@jasnell
jasnell requested a review from mcollinaAugust 7, 2019 00:56
Comment threadlib/_stream_writable.js Outdated

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.

As said in one of other PRs, these should be kept.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This change should not be here at all. Error from my side.

@mcollinamcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 7, 2019
@mcollina

Copy link
Copy Markdown
Member

Can you please avoid piling up commits from other PRs? Either open a "big" PR with multiple commits, or wait before the previous has landed.

Also, this needs a unit test.

@mcollinamcollina 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.

This should not land until the previous one has landed.

@ronag
ronagforce-pushed the stream-readable-error-emitted branch from 43093f5 to bb2a821CompareAugust 7, 2019 10:00
@ronag

ronag commented Aug 9, 2019

Copy link
Copy Markdown
MemberAuthor

replaced by #29058

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

Labels

semver-majorPRs that contain breaking changes and should be released in the next major version.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ronag@mcollina@nodejs-github-bot