stream: move to internal/streams by mcollina · Pull Request #35239 · nodejs/node

mcollina · 2020-09-17T08:11:57Z

Move all the streams constructors to internal/streams
and avoid a circular dependencies between the modules.

See: nodejs/readable-stream#348

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2020-09-17T08:21:47Z

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

ronag

Nice!

mcollina · 2020-09-17T09:53:54Z

@nodejs/testing is https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/16442/testReport/junit/(root)/test/parallel_test_bootstrap_modules/ a flake or something I should take of?

richardlau · 2020-09-17T10:22:40Z

@nodejs/testing is https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/16442/testReport/junit/(root)/test/parallel_test_bootstrap_modules/ a flake or something I should take of?

It's not a flake. This test detects when the modules loaded on startup have changed from the expected set. In this case it's the modules loaded by using workers. It's okay to change the expected set in the test -- the test is there to make sure this is a deliberate change (as is the case here) and to expose if anything is being unnecessarily loaded.

Move all the streams constructors to internal/streams and avoid a circular dependencies between the modules. See: nodejs/readable-stream#348

mcollina · 2020-09-17T14:21:47Z

For posterity, I run the failing test with: ./tools/test.py --worker parallel/test-bootstrap-modules

nodejs-github-bot · 2020-09-17T14:24:22Z

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

github-actions · 2020-09-21T08:23:13Z

Landed in 9c62e0e

Move all the streams constructors to internal/streams and avoid a circular dependencies between the modules. See: nodejs/readable-stream#348 PR-URL: #35239 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

ruyadorno · 2020-09-21T17:50:44Z

This does not land cleanly on v14.x-staging - Should it be backported to v14?

mcollina · 2020-09-21T21:07:01Z

Yes it should, I'll work on the backport.

mcollina · 2020-09-25T16:51:00Z

@ruyadorno backport in #35349.

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>

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>

ledbit · 2020-12-24T05:44:13Z

+Readable.ReadableState = ReadableState;
+
+const EE = require('events');
+const Stream = require('internal/streams/legacy');


Originally this line used to be
const Stream = require('stream');
which would cause full initialization of the 'Stream' library, which is required in line 97 to get access to Stream.Duplex. This is likely the root cause of #36615

mcollina · 2020-12-24T15:30:25Z

The fix is in #36618 and it's pretty straightforward.

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 requested review from jasnell, lpinca and ronag September 17, 2020 08:11

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 17, 2020

mcollina added stream Issues and PRs related to the stream subsystem. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 17, 2020

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 17, 2020

ronag approved these changes Sep 17, 2020
View reviewed changes

stream: move to internal/streams
f451ab0
Move all the streams constructors to internal/streams and avoid a circular dependencies between the modules. See: nodejs/readable-stream#348

mcollina force-pushed the move-streams-to-internal branch from 4d47eac to f451ab0Compare September 17, 2020 14:21

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 17, 2020

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 17, 2020

lpinca approved these changes Sep 17, 2020
View reviewed changes

watilde approved these changes Sep 17, 2020
View reviewed changes

vweevers mentioned this pull request Sep 19, 2020
Remove Circular dep that was not needed nodejs/readable-stream#441
Closed

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. dont-land-on-v10.x labels Sep 21, 2020

github-actionsBot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 21, 2020

github-actionsBot closed this Sep 21, 2020

mcollina deleted the move-streams-to-internal branch September 21, 2020 08:24

ruyadorno added the backport-requested-v14.x label Sep 21, 2020

kumavis mentioned this pull request Sep 22, 2020
Circular dependencies (prevents proper rollup) nodejs/readable-stream#348
Closed

mcollina mentioned this pull request Sep 25, 2020
[v14.x] stream: move to internal/streams #35349
Closed

BethGriggs mentioned this pull request Dec 10, 2020
v14.15.2 proposal #36476
Merged

ledbit mentioned this pull request Dec 24, 2020
gunzipSync DOA in Node 14.15.2 #36615
Closed

ledbit reviewed Dec 24, 2020
View reviewed changes

danielgindi mentioned this pull request Jan 25, 2021
feat(commonjs): Add support for circular dependencies rollup/plugins#658
Merged
9 tasks

targos added backported-to-v14.x and removed backport-requested-v14.x labels Apr 25, 2021

Navigation Menu

Uh oh!

Uh oh!

stream: move to internal/streams - #35239

stream: move to internal/streams#35239
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:move-streams-to-internal

mcollina commented Sep 17, 2020

Uh oh!

nodejs-github-bot commented Sep 17, 2020

Uh oh!

ronag left a comment

Uh oh!

mcollina commented Sep 17, 2020

Uh oh!

richardlau commented Sep 17, 2020

Uh oh!

mcollina commented Sep 17, 2020

Uh oh!

nodejs-github-bot commented Sep 17, 2020

Uh oh!

github-actionsBot commented Sep 21, 2020

Uh oh!

ruyadorno commented Sep 21, 2020

Uh oh!

mcollina commented Sep 21, 2020

Uh oh!

mcollina commented Sep 25, 2020

Uh oh!

ledbitDec 24, 2020

Uh oh!

mcollina commented Dec 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

Uh oh!

Conversation

mcollina commented Sep 17, 2020

Checklist

Uh oh!

nodejs-github-bot commented Sep 17, 2020

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Sep 17, 2020

Uh oh!

richardlau commented Sep 17, 2020

Uh oh!

mcollina commented Sep 17, 2020

Uh oh!

nodejs-github-bot commented Sep 17, 2020

Uh oh!

github-actionsBot commented Sep 21, 2020

Uh oh!

ruyadorno commented Sep 21, 2020

Uh oh!

mcollina commented Sep 21, 2020

Uh oh!

mcollina commented Sep 25, 2020

Uh oh!

ledbitDec 24, 2020

Choose a reason for hiding this comment

Uh oh!

mcollina commented Dec 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants