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

Partially revert "stream: reduce internal usage of public require of util" #29475

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Sep 6, 2019

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: #26698
Refs: nodejs/readable-stream#416

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

…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
@addaleax addaleax requested a review from mcollina September 6, 2019 17:22
@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Sep 6, 2019
Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

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

I'm unclear as to what this accomplishes exactly and the support it is providing.

@addaleax
Copy link
Member Author

addaleax commented Sep 6, 2019

@bmeck It should make maintaining readable-stream easier, and it makes the fallback code in lib/streams.js make any sense at all (in its current form, the internal require would always throw in userland, rendering the rest of the try/catch useless).

If that isn’t an acceptable reason for this PR in your eyes, please close it, as I don’t think it’s worth spending much time arguing about this relatively minor change.

@targos
Copy link
Member

targos commented Sep 6, 2019

@addaleax I don't exactly understand how this works, because the code in lib/stream.js is not part of the readable-stream module

@bmeck
Copy link
Member

bmeck commented Sep 6, 2019

@addaleax I think thats fine, just please add a disclaimer somewhere to that effect in the actual file itself to prevent some thrashing we are seeing on moving things between internal/public APIs. I would like to PR removing code for various fallback types like process.binding as we move forward with LTS cycle when we have purely public APIs we haven't deprecated though, but thats a different PR.

@addaleax
Copy link
Member Author

addaleax commented Sep 6, 2019

Hm right, it looks like the fallback code in streams.js might not be used by readable-stream at all … in that case we can probably remove it altogether? Can somebody from @nodejs/streams confirm?

@mcollina
Copy link
Member

mcollina commented Sep 8, 2019

Yes, it’s not used in the active line. You can remove it.

@addaleax addaleax closed this Sep 9, 2019
@addaleax addaleax deleted the undo-26698-streams branch September 9, 2019 21:55
addaleax added a commit to addaleax/node that referenced this pull request Sep 9, 2019
The fallback code is no longer used when exporting to readable-stream.

Refs: nodejs#29475
mcollina pushed a commit that referenced this pull request Sep 12, 2019
The fallback code is no longer used when exporting to readable-stream.

Refs: #29475

PR-URL: #29514
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
targos pushed a commit that referenced this pull request Sep 20, 2019
The fallback code is no longer used when exporting to readable-stream.

Refs: #29475

PR-URL: #29514
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
The fallback code is no longer used when exporting to readable-stream.

Refs: #29475

PR-URL: #29514
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
@addaleax addaleax mentioned this pull request Oct 20, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants