GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — stream: move to internal/streams by mcollina · Pull Request #35239 · nodejs/node

Conversation

@mcollina

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lundibundi, bricss, ronag, rickyes, kumavis, and qm3ster reacted with heart emoji
All reactions
  • ❤️ 6 reactions
@mcollina
mcollina requested review from jasnell, lpinca and ronag September 17, 2020 08:11
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs involving general changes in the lib/ or src/ directories. label Sep 17, 2020
@mcollina mcollina added stream Issues and PRs related to Node.js streams. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 17, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 17, 2020
@nodejs-github-bot

nodejs-github-bot commented Sep 17, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ronag
ronag approved these changes Sep 17, 2020
View reviewed changes

@ronag ronag left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nice!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@richardlau

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina
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
mcollina force-pushed the move-streams-to-internal branch from 4d47eac to f451ab0 Compare September 17, 2020 14:21
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 17, 2020
@mcollina

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 17, 2020
@nodejs-github-bot

nodejs-github-bot commented Sep 17, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca
lpinca approved these changes Sep 17, 2020
View reviewed changes
watilde
watilde approved these changes Sep 17, 2020
View reviewed changes
@vweevers vweevers mentioned this pull request Sep 19, 2020
Remove Circular dep that was not needed nodejs/readable-stream#441
Closed
@mcollina mcollina added commit-queue PRs queued for automated landing through the Commit Queue. dont-land-on-v10.x labels Sep 21, 2020
@github-actions github-actions Bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 21, 2020
@github-actions

github-actions Bot commented Sep 21, 2020

Copy link
Copy Markdown
Contributor

Landed in 9c62e0e
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot closed this Sep 21, 2020
nodejs-github-bot pushed a commit that referenced this pull request Sep 21, 2020
@mcollina @nodejs-github-bot
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
@mcollina
mcollina deleted the move-streams-to-internal branch September 21, 2020 08:24
@ruyadorno ruyadorno added the backport-requested-v14.x label Sep 21, 2020
@ruyadorno

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Sep 21, 2020

Copy link
Copy Markdown
Member Author

Yes it should, I'll work on the backport.
ruyadorno reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumavis kumavis mentioned this pull request Sep 22, 2020
Circular dependencies (prevents proper rollup) nodejs/readable-stream#348
Closed
@mcollina mcollina mentioned this pull request Sep 25, 2020
[v14.x] stream: move to internal/streams #35349
Closed
@mcollina

mcollina commented Sep 25, 2020

Copy link
Copy Markdown
Member Author

@ruyadorno backport in #35349.
ruyadorno reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina added a commit to mcollina/node that referenced this pull request Dec 2, 2020
@mcollina
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 2, 2020
@mcollina @BethGriggs
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
v14.15.2 proposal #36476
Merged
@ledbit ledbit mentioned this pull request Dec 24, 2020
gunzipSync DOA in Node 14.15.2 #36615
Closed
ledbit
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 ledbit Dec 24, 2020

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina

mcollina commented Dec 24, 2020

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@mcollina @joesepi
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
@danielgindi danielgindi mentioned this pull request Jan 25, 2021
feat(commonjs): Add support for circular dependencies rollup/plugins#658
Merged
4 of 9 tasks
@targos targos added backported-to-v14.x and removed backport-requested-v14.x labels Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

lib / src Issues and PRs involving general changes in the lib/ or src/ directories. stream Issues and PRs related to Node.js streams.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@mcollina @nodejs-github-bot @richardlau @ruyadorno @lpinca @watilde @ronag @ledbit @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.