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

stream: reduce internal usage of public require of util #26698

Closed
wants to merge 1 commit into from
Closed

stream: reduce internal usage of public require of util #26698

wants to merge 1 commit into from

Conversation

BeniCheni
Copy link
Contributor

@BeniCheni BeniCheni commented Mar 16, 2019

Refs #26546

Reduce internal usage of public require of util in the scope of stream.

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Mar 16, 2019
@BridgeAR
Copy link
Member

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 17, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 21, 2019
PR-URL: nodejs#26698
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member

Landed in c97851d 🎉

@BridgeAR BridgeAR closed this Mar 21, 2019
@BeniCheni BeniCheni deleted the remove-internal-require-stream branch March 21, 2019 23:59
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
PR-URL: nodejs#26698
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Mar 27, 2019
PR-URL: #26698
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mcollina
Copy link
Member

mcollina commented May 26, 2019

Why @nodejs/streams was not tagged on this one? I would have opposed this change landing.

@addaleax
Copy link
Member

addaleax commented Sep 6, 2019

Yes, this should not have happened, see nodejs/readable-stream#416 (comment). I’ll open a revert PR for the streams parts.

addaleax added a commit to addaleax/node that referenced this pull request Sep 6, 2019
…util"

This partially reverts commit c97851d.

Streams code should ideally require on public APIs as much as possible,
because it is exported as readable-stream.

Refs: nodejs#26698
Refs: nodejs/readable-stream#416
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants