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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

wa-Nadoo
Copy link

… functions

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

@nodejs-github-bot
Copy link
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
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

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@wa-Nadoo
Copy link
Author

I am working on fix

@wa-Nadoo
Copy link
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. needs-benchmark-ci PR that need a benchmark CI run. labels Oct 26, 2023
@rluvaton
Copy link
Member

rluvaton commented Oct 26, 2023 •
edited
Loading

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. label Oct 26, 2023
@nodejs-github-bot
Copy link
Collaborator

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

rluvaton
rluvaton reviewed Oct 26, 2023
View reviewed changes
lib/internal/streams/readable.js Show resolved Hide resolved
lib/internal/streams/readable.js Outdated Show resolved Hide resolved
lib/internal/streams/readable.js Outdated Show resolved Hide resolved
ronag
ronag requested changes Oct 26, 2023
View reviewed changes
Copy link
Member

@ronag ronag left a comment

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 ronag and rluvaton October 27, 2023 01:36
@wa-Nadoo
Copy link
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
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 PR that need a benchmark CI run. needs-ci PRs that need a full CI run.
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