stream: move to internal/streams #35239
Conversation
nodejs-github-bot
added
the
lib / src
mcollina
added
stream
github-actions
bot
removed
the
request-ci
|
@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
force-pushed
the
move-streams-to-internal
branch
from
4d47eac
to
f451ab0
Compare
|
For posterity, I run the failing test with: |
github-actions
bot
removed
the
request-ci
mcollina
added
commit-queue
github-actions
bot
removed
the
commit-queue
|
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>
|
This does not land cleanly on |
|
Yes it should, I'll work on the backport. |
|
@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>
| Readable.ReadableState = ReadableState; | ||
|
|
||
| const EE = require('events'); | ||
| const Stream = require('internal/streams/legacy'); |
There was a problem hiding this comment.
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
|
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>
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), orvcbuild test(Windows) passes