stream: refactor streams by VoltrexKeyva · Pull Request #41871 · nodejs/node

VoltrexKeyva · 2022-02-06T04:39:29Z

Use the standard for loop style instead of for..of for consistency.
Refactor to use more validators.
Avoid using deprecated APIs.

nodejs-github-bot · 2022-02-06T04:39:33Z

Review requested:

@nodejs/streams

mcollina

lgtm

lib/internal/streams/readable.js

aduh95 · 2022-02-06T11:26:04Z

Can we use a less scary commit message? 😅

VoltrexKeyva · 2022-02-06T12:12:01Z

Can we use a less scary commit message? 😅

I'm unsure what you mean by that, do you mean the commit message title? If so, what do you suggest we use instead? (Or if that was a joke :^))

nodejs-github-bot · 2022-02-06T13:33:59Z

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

aduh95 · 2022-02-06T18:07:52Z

If so, what do you suggest we use instead?

Regarding the commit title, I suggest: stream: refactor to use more validators and avoid using deprecated APIs.

refactor streams is a bit too broad, and could imply the commit is doing a complete refactor of the stream API.

lib/internal/streams/duplex.js

VoltrexKeyva · 2022-02-06T20:29:47Z

If so, what do you suggest we use instead?

Regarding the commit title, I suggest: stream: refactor to use more validators and avoid using deprecated APIs.

refactor streams is a bit too broad, and could imply the commit is doing a complete refactor of the stream API.

That exceeds the 50 character limit for commit message titles (because of our linters), and these changes touch upon multiple files, which is why I chose to use refactor streams, I don't think there's a replacement for the current commit message title that explains the changes without exceeding the character limit?

aduh95 · 2022-02-06T21:11:22Z

That exceeds the 50 character limit for commit message titles (because of our linters)

It doesn't exceed our 72 character limit so we're good. Another (better?) solution would be to split this into three separate commits.

Mesteery · 2022-02-06T21:13:27Z

Maybe you can split this commit into 3 commits (+ commit-queue-rebase), for example:

stream: refactor to use more validators
stream: use for loop instead of for-of loop (including the block statement removal and aduh's suggestion)
stream: replace the use of static listenerCount

(I'm too slow 👀😄)

lib/internal/streams/operators.js

Use the standard `for` loop style instead of `for..of` for consistency. PR-URL: #41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Use more validators where appropriate for consistency. PR-URL: #41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Avoid usage of the `events.listenerCount()` method as it is deprecated. PR-URL: #41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Use the standard `for` loop style instead of `for..of` for consistency. PR-URL: nodejs#41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Use more validators where appropriate for consistency. PR-URL: nodejs#41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Avoid usage of the `events.listenerCount()` method as it is deprecated. PR-URL: nodejs#41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Use the standard `for` loop style instead of `for..of` for consistency. PR-URL: nodejs#41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Use more validators where appropriate for consistency. PR-URL: nodejs#41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Avoid usage of the `events.listenerCount()` method as it is deprecated. PR-URL: nodejs#41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Use the standard `for` loop style instead of `for..of` for consistency. PR-URL: #41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Use more validators where appropriate for consistency. PR-URL: #41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Avoid usage of the `events.listenerCount()` method as it is deprecated. PR-URL: #41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

danielleadams · 2022-04-11T01:57:59Z

@VoltrexMaster this doesn't land cleanly in 16.x - do you mind making a backport PR?

VoltrexKeyva · 2022-04-11T02:02:01Z

@VoltrexMaster this doesn't land cleanly in 16.x - do you mind making a backport PR?

What's blocking it from landing cleanly? I don't see anything that should be changed to land on v16.x

Use the standard `for` loop style instead of `for..of` for consistency. PR-URL: #41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Use more validators where appropriate for consistency. PR-URL: #41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Avoid usage of the `events.listenerCount()` method as it is deprecated. PR-URL: #41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Use the standard `for` loop style instead of `for..of` for consistency. PR-URL: #41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Use more validators where appropriate for consistency. PR-URL: #41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Avoid usage of the `events.listenerCount()` method as it is deprecated. PR-URL: #41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Use the standard `for` loop style instead of `for..of` for consistency. PR-URL: nodejs/node#41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Use more validators where appropriate for consistency. PR-URL: nodejs/node#41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Avoid usage of the `events.listenerCount()` method as it is deprecated. PR-URL: nodejs/node#41871 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 6, 2022

VoltrexKeyva added the stream Issues and PRs related to the stream subsystem. label Feb 6, 2022

VoltrexKeyva force-pushed the refactor-streams branch from f764be1 to bc719a3 Compare February 6, 2022 04:46

mcollina approved these changes Feb 6, 2022

View reviewed changes

benjamingr approved these changes Feb 6, 2022

View reviewed changes

ronag reviewed Feb 6, 2022

View reviewed changes

lib/internal/streams/readable.js Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

ronag approved these changes Feb 6, 2022

View reviewed changes

VoltrexKeyva force-pushed the refactor-streams branch from bc719a3 to 66f334a Compare February 6, 2022 12:10

RaisinTen approved these changes Feb 6, 2022

View reviewed changes

jasnell approved these changes Feb 6, 2022

View reviewed changes

aduh95 approved these changes Feb 6, 2022

View reviewed changes

lib/internal/streams/duplex.js Outdated Show resolved Hide resolved

Mesteery approved these changes Feb 6, 2022

View reviewed changes

Mesteery closed this Feb 6, 2022

Mesteery reopened this Feb 6, 2022

VoltrexKeyva force-pushed the refactor-streams branch from 66f334a to 23c31e7 Compare February 6, 2022 21:45

ronag approved these changes Feb 6, 2022

View reviewed changes

aduh95 reviewed Feb 6, 2022

View reviewed changes

lib/internal/streams/operators.js Show resolved Hide resolved

github-actions bot mentioned this pull request Feb 7, 2022

CI Reliability 2022-02-07 nodejs/reliability#195

Open

29 tasks

VoltrexKeyva force-pushed the refactor-streams branch 2 times, most recently from 1251528 to 57e292d Compare February 7, 2022 07:41

VoltrexKeyva deleted the refactor-streams branch February 11, 2022 11:52

This was referenced Feb 12, 2022

CI Reliability 2022-02-12 nodejs/reliability#200

Open

CI Reliability 2022-02-13 nodejs/reliability#201

Open

bengl mentioned this pull request Feb 21, 2022

v17.6.0 release proposal #42072

Merged

danielleadams added the backport-requested-v16.x label Apr 11, 2022

targos removed the backport-requested-v16.x label Jul 12, 2022

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

Uh oh!

Conversation

VoltrexKeyva commented Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 6, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

aduh95 commented Feb 6, 2022

Uh oh!

VoltrexKeyva commented Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 6, 2022

Uh oh!

aduh95 commented Feb 6, 2022

Uh oh!

Uh oh!

VoltrexKeyva commented Feb 6, 2022

Uh oh!

aduh95 commented Feb 6, 2022

Uh oh!

Mesteery commented Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

danielleadams commented Apr 11, 2022

Uh oh!

VoltrexKeyva commented Apr 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Comments

VoltrexKeyva commented Feb 6, 2022 •

edited

Loading

VoltrexKeyva commented Feb 6, 2022 •

edited

Loading

Mesteery commented Feb 6, 2022 •

edited

Loading