GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — stream: consolidate common code from readable push and unshift helper… by wa-Nadoo · Pull Request #50402 · nodejs/node

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Oct 26, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
ronag
ronag approved these changes Oct 26, 2023
View reviewed changes
mcollina
mcollina approved these changes Oct 26, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@wa-Nadoo

wa-Nadoo commented Oct 26, 2023

Copy link
Copy Markdown
Author

I am working on fix
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@wa-Nadoo

wa-Nadoo commented Oct 26, 2023

Copy link
Copy Markdown
Author

Fixed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Uh oh!

There was an error while loading. Please reload this page.

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
wa-Nadoo reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rluvaton
rluvaton reviewed Oct 26, 2023
View reviewed changes
Comment thread lib/internal/streams/readable.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/streams/readable.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/streams/readable.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
ronag
ronag requested changes Oct 26, 2023
View reviewed changes

@ronag ronag left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rluvaton and wa-Nadoo reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@github-actions github-actions Bot mentioned this pull request Oct 27, 2023
CI Reliability 2023-10-27 nodejs/reliability#699
Open
19 tasks
@wa-Nadoo
stream: consolidate common code from push and unshift helper functions
1ece82b
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Oct 28, 2023
CI Reliability 2023-10-28 nodejs/reliability#700
Open
CI Reliability 2023-10-29 nodejs/reliability#701
Open
CI Reliability 2023-10-30 nodejs/reliability#702
Open
CI Reliability 2023-10-31 nodejs/reliability#703
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@wa-Nadoo @nodejs-github-bot @rluvaton @mcollina @ronag
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.