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.

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

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

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Sep 25, 2020

Back port of #35239 to v14.x staging.

@nodejs-github-bot 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 mcollina added the stream Issues and PRs related to the stream subsystem. label Sep 25, 2020
@mcollina mcollina mentioned this pull request Sep 25, 2020
3 tasks
@mcollina mcollina requested a review from ruyadorno September 25, 2020 16:51
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.

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
Copy link
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
Copy link
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
Copy link
Member

ronag commented Sep 29, 2020

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 mcollina closed this Sep 29, 2020
@mcollina mcollina reopened this Oct 22, 2020
@mcollina
Copy link
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 mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 22, 2020
@github-actions 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
Copy link
Collaborator

@mcollina
Copy link
Member Author

also cc @MylesBorins

@BethGriggs
Copy link
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
Copy link
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
Copy link
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
Copy link
Member Author

Please let's land this asap, thanks.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

ERR_TRANSFORM_ALREADY_TRANSFORMING,
ERR_TRANSFORM_WITH_LENGTH_0
} = require('internal/errors').codes;
const Duplex = require('_stream_duplex');
Copy link
Member

Choose a reason for hiding this comment

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

Should this be?

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually yes! Good spot

Copy link
Member Author

Choose a reason for hiding this comment

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

done

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 mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 2, 2020
@github-actions 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
Copy link
Collaborator

nodejs-github-bot commented Dec 2, 2020

BethGriggs pushed a commit that referenced this pull request Dec 2, 2020
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
Copy link
Member

Landed in fb14acb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

9 participants