FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v14.x] stream: move to internal/streams by mcollina · Pull Request #35349 · nodejs/node · GitHub

/ node Public

[v14.x] stream: move to internal/streams - #35349

Closed
mcollina wants to merge 1 commit into
nodejs:v14.x-stagingfrom
mcollina:backport-35239
ConversationCommits (1)ChecksFiles changed
Closed

[v14.x] stream: move to internal/streams#35349
mcollina wants to merge 1 commit into
nodejs:v14.x-stagingfrom
mcollina:backport-35239

Conversation

mcollina commented Sep 25, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Back port of #35239 to v14.x staging.

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v14.x labels Sep 25, 2020
mcollina added the stream Issues and PRs related to the stream subsystem. label Sep 25, 2020
mcollina mentioned this pull request Sep 25, 2020
Closed
3 tasks
mcollina requested a review from ruyadorno September 25, 2020 16:51
ronag approved these changes Sep 25, 2020
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. Could I have until Tuesday to try and sort out any other pending streams backports before we land this? Would make life easier.

MylesBorins commented Sep 29, 2020

Copy link
Copy Markdown
Contributor

I'm in the process of preparing the 14.x release. Sounds like there are some other commits that need to land first? If this is ready to land and definitely should go out today please let me know ASAP

mcollina commented Sep 29, 2020

Copy link
Copy Markdown
Member Author

I would prefer this to ship before v14 goes LTS as it would help with readable-stream. Overall, it can wait a week or two.

@ronag which PRs? Note that I had to redo the work for the backport because of the conflicts, so you might want to do a single backport PR with a few commits in.

ronag commented Sep 29, 2020

Copy link
Copy Markdown
Member

so you might want to do a single backport PR with a few commits in.

Ah let's do it like that. I can open backports later when I get the chance. A little short on time at the moment.

mcollina closed this Sep 29, 2020
mcollina reopened this Oct 22, 2020

mcollina commented Oct 22, 2020

Copy link
Copy Markdown
Member Author

This was not backported yet by any other PR.. I think it's really important to have this in the latest v14.x release before LTS.

cc @ronag @BethGriggs @ronag.

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 22, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 22, 2020

nodejs-github-bot commented Oct 22, 2020

Copy link
Copy Markdown
Collaborator

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

mcollina commented Oct 22, 2020

Copy link
Copy Markdown
Member Author

also cc @MylesBorins

BethGriggs commented Oct 27, 2020

Copy link
Copy Markdown
Member

The v14.15.0 LTS transition release (#35746) is due to go out today.

Although it would have been ideal to land this PR before LTS, do you think it can still land post-LTS (in the next release)?

richardlau commented Oct 27, 2020

Copy link
Copy Markdown
Member

The v14.15.0 LTS transition release (#35746) is due to go out today.

Although it would have been ideal to land this PR before LTS, do you think it can still land post-LTS (in the next release)?

Just to be clear, the last Node.js 14.x before LTS was 14.14.0 on 15 Oct. The LTS transition release is going to happen today, releases have already been successfully built and awaiting promotion (which I'll do during this afternoon's Release WG mentoring session). So this can only land in 14.x post-LTS.

BethGriggs commented Oct 27, 2020

Copy link
Copy Markdown
Member

So this can only land in 14.x post-LTS.

To be clear, my question is should this still land now that it will have to be during LTS?

mcollina commented Oct 27, 2020

Copy link
Copy Markdown
Member Author

Please let's land this asap, thanks.

This comment has been minimized.

Sign in to view

nodejs-github-bot commented Dec 2, 2020

Copy link
Copy Markdown
Collaborator

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

BethGriggs reviewed Dec 2, 2020
View reviewed changes
Comment thread lib/internal/streams/transform.js Outdated

BethGriggs Dec 2, 2020

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should this be?

Suggested change
const Duplex = require('_stream_duplex');
const Duplex = require('internal/streams/duplex');

mcollina Dec 2, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Actually yes! Good spot

mcollina Dec 2, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

done

stream: move to internal/streams
6f99bd1 
Move all the streams constructors to internal/streams
and avoid a circular dependencies between the modules.

See: nodejs/readable-stream#348

PR-URL: nodejs#35239
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 2, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 2, 2020

nodejs-github-bot commented Dec 2, 2020 •
edited by BethGriggs
Loading

Copy link
Copy Markdown
Collaborator

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

BethGriggs pushed a commit that referenced this pull request Dec 2, 2020
stream: move to internal/streams
fb14acb 
Move all the streams constructors to internal/streams
and avoid a circular dependencies between the modules.

See: nodejs/readable-stream#348

PR-URL: #35239
Backport-PR-URL: #35349
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

BethGriggs commented Dec 2, 2020

Copy link
Copy Markdown
Member

Landed in fb14acb

BethGriggs closed this Dec 2, 2020
BethGriggs mentioned this pull request Dec 7, 2020
Closed
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ruyadorno ruyadorno approved these changes
lpinca lpinca approved these changes
watilde watilde approved these changes
ronag ronag approved these changes
BethGriggs BethGriggs approved these changes
targos Awaiting requested review from targos

Assignees

No one assigned

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL