Visitar URL original
stream: consolidate common code from readable push and unshift helper… by wa-Nadoo · Pull Request #50402 · nodejs/node · GitHub
Skip to content

stream: consolidate common code from readable push and unshift helper… - #50402

Closed
wa-Nadoo wants to merge 1 commit into
nodejs:mainfrom
wa-Nadoo:readable-deduplication
Closed

stream: consolidate common code from readable push and unshift helper…#50402
wa-Nadoo wants to merge 1 commit into
nodejs:mainfrom
wa-Nadoo:readable-deduplication

Conversation

@wa-Nadoo

@wa-Nadoo wa-Nadoo commented Oct 26, 2023

Copy link
Copy Markdown

… functions

#50173 brings too many code duplication. Trying to reduce it a bit.

@nodejs-github-bot

nodejs-github-bot commented Oct 26, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 26, 2023
@wa-Nadoo
wa-Nadoo force-pushed the readable-deduplication branch from 8612f4a to a3cb8ba Compare October 26, 2023 03:02
mcollina
mcollina approved these changes Oct 26, 2023
View reviewed changes

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 26, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 26, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 26, 2023

Copy link
Copy Markdown
Collaborator

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

@wa-Nadoo

wa-Nadoo commented Oct 26, 2023

Copy link
Copy Markdown
Author

I am working on fix

@wa-Nadoo

wa-Nadoo commented Oct 26, 2023

Copy link
Copy Markdown
Author

Fixed

@wa-Nadoo
wa-Nadoo requested review from mcollina and ronag October 26, 2023 17:15
@rluvaton rluvaton added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. needs-benchmark-ci PRs that need a benchmark CI run. labels Oct 26, 2023
@rluvaton

rluvaton commented Oct 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Hey, I'm the author of the original PR

I'm pretty sure this will break this change the behavior as you move code from the middle of the function to the start

@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 26, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 26, 2023

Copy link
Copy Markdown
Collaborator

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

rluvaton
rluvaton reviewed Oct 26, 2023
View reviewed changes
Comment thread lib/internal/streams/readable.js
Comment thread lib/internal/streams/readable.js Outdated
Comment thread lib/internal/streams/readable.js Outdated
ronag
ronag requested changes Oct 26, 2023
View reviewed changes

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

I think this changes behavior. Not only deduplicating code.

@github-actions github-actions Bot mentioned this pull request Oct 27, 2023
Open
19 tasks
@wa-Nadoo
wa-Nadoo force-pushed the readable-deduplication branch from b5e9176 to 1ece82b Compare October 27, 2023 01:35
@wa-Nadoo
wa-Nadoo requested review from rluvaton and ronag October 27, 2023 01:36
@wa-Nadoo

wa-Nadoo commented Oct 27, 2023

Copy link
Copy Markdown
Author

From my understanding of data flow inside Readable stream, the changes would be safe. I was wrong. All breaking changes are already reverted.

This was referenced Oct 28, 2023
Open
Open
Open
Open
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actions github-actions Bot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
@wa-Nadoo wa-Nadoo closed this Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina Awaiting requested review from mcollina
@ronag ronag Awaiting requested review from ronag
@rluvaton rluvaton Awaiting requested review from rluvaton

Assignees

No one assigned

Labels

needs-benchmark-ci PRs that need a benchmark CI run. needs-ci PRs that need a full CI run. stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wa-Nadoo @nodejs-github-bot @rluvaton @mcollina @ronag