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

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

Review requested:

@nodejs/streams

lgtm

Can we use a less scary commit message? 😅

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 :^))

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

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.

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?

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.

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 👀😄)

Use the standard `for` loop style instead of `for..of` for consistency. PR-URL: #41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Use more validators where appropriate for consistency. PR-URL: #41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Avoid usage of the `events.listenerCount()` method as it is deprecated. PR-URL: #41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Use the standard `for` loop style instead of `for..of` for consistency. PR-URL: nodejs#41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Use more validators where appropriate for consistency. PR-URL: nodejs#41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Avoid usage of the `events.listenerCount()` method as it is deprecated. PR-URL: nodejs#41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Use the standard `for` loop style instead of `for..of` for consistency. PR-URL: nodejs#41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Use more validators where appropriate for consistency. PR-URL: nodejs#41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Avoid usage of the `events.listenerCount()` method as it is deprecated. PR-URL: nodejs#41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Use the standard `for` loop style instead of `for..of` for consistency. PR-URL: #41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Use more validators where appropriate for consistency. PR-URL: #41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Avoid usage of the `events.listenerCount()` method as it is deprecated. PR-URL: #41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

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

@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 <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Use more validators where appropriate for consistency. PR-URL: #41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Avoid usage of the `events.listenerCount()` method as it is deprecated. PR-URL: #41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Use the standard `for` loop style instead of `for..of` for consistency. PR-URL: #41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Use more validators where appropriate for consistency. PR-URL: #41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Avoid usage of the `events.listenerCount()` method as it is deprecated. PR-URL: #41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Use the standard `for` loop style instead of `for..of` for consistency. PR-URL: nodejs/node#41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Use more validators where appropriate for consistency. PR-URL: nodejs/node#41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Avoid usage of the `events.listenerCount()` method as it is deprecated. PR-URL: nodejs/node#41871 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

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

Comment thread lib/internal/streams/readable.js Outdated

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

Comment thread lib/internal/streams/duplex.js Outdated

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

Comment thread lib/internal/streams/operators.js Outdated

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

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

Uh oh!

Uh oh!

stream: refactor streams - #41871

stream: refactor streams#41871VoltrexKeyva wants to merge 3 commits intonodejs:masternodejs/node:masterfrom VoltrexKeyva:refactor-streamsVoltrexKeyva/node:refactor-streamsCopy head branch name to clipboard

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

stream: refactor streams#41871
VoltrexKeyva wants to merge 3 commits into
nodejs:masterfrom
VoltrexKeyva:refactor-streams

VoltrexKeyva commented Feb 6, 2022 •
edited
Loading

VoltrexKeyva commented Feb 6, 2022 •
edited
Loading

Mesteery commented Feb 6, 2022 •
edited
Loading