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

stream: fix pipeline pump #39006

Closed
wants to merge 13 commits into from
Closed

Conversation

ronag
Copy link
Member

@ronag ronag commented Jun 11, 2021

Refs: #39005

@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Jun 11, 2021
@ronag ronag requested a review from mcollina June 11, 2021 09:16
@ronag
Copy link
Member Author

ronag commented Jun 11, 2021

Needs tests.

@github-actions github-actions bot added the needs-ci PRs that need a full CI run. label Jun 11, 2021
@ronag ronag force-pushed the fix-pipeline-pump branch from 1b18705 to 963f536 Compare June 11, 2021 09:58
@VoltrexKeyva

This comment has been minimized.

@VoltrexKeyva

This comment has been minimized.

@ronag
Copy link
Member Author

ronag commented Jun 11, 2021

@VoltrexMaster Thanks for the help. Why not make github change suggestions? That way I can just commit your changes directly.

Screen Shot 2021-06-11 at 14 18 30

In general it's better to review the code in the code instead of in comments.

@VoltrexKeyva
Copy link
Member

@ronag yea sorry, I already know how those work; just placed those in a comment as github doesn't allow suggestions in unchanged lines which for example the const { line, wanted to be more specific if you know what I mean 😅

lib/internal/streams/pipeline.js Outdated Show resolved Hide resolved
lib/internal/streams/pipeline.js Outdated Show resolved Hide resolved
lib/internal/streams/pipeline.js Outdated Show resolved Hide resolved
ronag and others added 2 commits June 11, 2021 15:24
lib/internal/streams/pipeline.js Outdated Show resolved Hide resolved
lib/internal/streams/pipeline.js Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Can you please add a unit test?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 12, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 12, 2021

@ronag
Copy link
Member Author

ronag commented Jun 12, 2021

@nodejs/streams

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@jasnell
Copy link
Member

jasnell commented Jun 14, 2021

Landed in bdcb738

@jasnell jasnell closed this Jun 14, 2021
jasnell pushed a commit that referenced this pull request Jun 14, 2021
Refs: #39005

PR-URL: #39006
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this pull request Jun 15, 2021
Refs: #39005

PR-URL: #39006
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@danielleadams danielleadams mentioned this pull request Jun 15, 2021
danielleadams pushed a commit that referenced this pull request Jun 17, 2021
Refs: #39005

PR-URL: #39006
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@richardlau
Copy link
Member

This doesn't land cleanly on v14.x-staging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
Development

Successfully merging this pull request may close these issues.

8 participants