FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

stream: move to internal/streams - #35239

Closed
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:move-streams-to-internal
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

mcollina commented Sep 17, 2020

Copy link
Copy Markdown
Member

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 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-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 17, 2020

nodejs-github-bot commented Sep 17, 2020

Copy link
Copy Markdown
Collaborator

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

ronag approved these changes Sep 17, 2020
View reviewed changes

ronag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nice!

mcollina commented Sep 17, 2020

Copy link
Copy Markdown
Member Author

@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 commented Sep 17, 2020

Copy link
Copy Markdown
Member

@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.

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 f451ab0 Compare 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

mcollina commented Sep 17, 2020

Copy link
Copy Markdown
Member Author

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

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

nodejs-github-bot commented Sep 17, 2020

Copy link
Copy Markdown
Collaborator

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

vweevers mentioned this pull request Sep 19, 2020
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-actions Bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 21, 2020

github-actions Bot commented Sep 21, 2020

Copy link
Copy Markdown
Contributor

Landed in 9c62e0e

github-actions Bot closed this Sep 21, 2020
nodejs-github-bot pushed a commit that referenced this pull request Sep 21, 2020
stream: move to internal/streams
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>
mcollina deleted the move-streams-to-internal branch September 21, 2020 08:24

ruyadorno commented Sep 21, 2020

Copy link
Copy Markdown
Member

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

mcollina commented Sep 21, 2020

Copy link
Copy Markdown
Member Author

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

kumavis mentioned this pull request Sep 22, 2020
Closed
mcollina mentioned this pull request Sep 25, 2020
Closed

mcollina commented Sep 25, 2020

Copy link
Copy Markdown
Member Author

@ruyadorno backport in #35349.

mcollina added a commit to mcollina/node that referenced this pull request Dec 2, 2020
stream: move to internal/streams
6f99bd1 
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>
BethGriggs pushed a commit that referenced this pull request Dec 2, 2020
stream: move to internal/streams
fb14acb 
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 mentioned this pull request Dec 10, 2020
Merged
ledbit mentioned this pull request Dec 24, 2020
Closed
ledbit reviewed Dec 24, 2020
View reviewed changes
Comment thread lib/internal/streams/readable.js
Readable.ReadableState = ReadableState;

const EE = require('events');
const Stream = require('internal/streams/legacy');

ledbit Dec 24, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Dec 24, 2020

Copy link
Copy Markdown
Member Author

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

joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
stream: move to internal/streams
e49f28a 
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>
danielgindi mentioned this pull request Jan 25, 2021
Merged
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca approved these changes
watilde watilde approved these changes
ronag ronag approved these changes
jasnell Awaiting requested review from jasnell
+1 more reviewer
ledbit ledbit left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL