GeezerVison

View reviewed changes

MoLow 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

ronag approved these changes

View reviewed changes

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

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

nodejs-github-bot commented Oct 15, 2023

Copy link

Copy Markdown

Collaborator

All reactions

Sorry, something went wrong.

Uh oh!

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

himself65 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

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

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

nodejs-github-bot commented Oct 15, 2023

Copy link

Copy Markdown

Collaborator

All reactions

Sorry, something went wrong.

Uh oh!

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

MoLow added author ready

PRs with CI started, the required approvals, and no outstanding review comments.

commit-queue PRs queued for automated landing through the Commit Queue. labels

aymen94 approved these changes

View reviewed changes

benjamingr approved these changes

View reviewed changes

koshic reviewed

node/lib/internal/streams/duplex.js

View reviewed changes

Comment thread

lib/internal/streams/duplexify.js

		const { value, write, final, destroy } = fromAsyncGen(body);

		// Body might be a constructor function instead of an async generator function.
		if (isDuplexNodeStream(value)) {

koshic Oct 16, 2023

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.

should we reflect this in the docs?

Sorry, something went wrong.

Uh oh!

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

All reactions

himself65 Oct 16, 2023

Copy link

Copy Markdown

Member Author

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.

Hard to say, since this is for internal use and the way of init is not decently

Sorry, something went wrong.

Uh oh!

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

All reactions

koshic Oct 16, 2023

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.

It's used for public API Duplex.from, whose behavior will also be slightly changed

Lines 142 to 147 in 356b4a2

	Duplex.from = function(body) {
	if (!duplexify) {
	duplexify = require('internal/streams/duplexify');
	}
	return duplexify(body, 'body');
	};

Sorry, something went wrong.

Uh oh!

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

All reactions

himself65 Oct 16, 2023

Copy link

Copy Markdown

Member Author

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.

Duplex.from is still an experimental API. I will put it into doc later

Sorry, something went wrong.

Uh oh!

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

All reactions

koshic Oct 16, 2023

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.

Stable pipeline also affected

node/lib/internal/streams/pipeline.js

Lines 288 to 298 in 4032ad5

	if (typeof stream === 'function') {
	ret = stream({ signal });
	if (!isIterable(ret)) {
	throw new ERR_INVALID_RETURN_VALUE(
	'Iterable, AsyncIterable or Stream', 'source', ret);
	}
	} else if (isIterable(stream) \|\| isReadableNodeStream(stream) \|\| isTransformStream(stream)) {
	ret = stream;
	} else {
	ret = Duplex.from(stream);
	}

Sorry, something went wrong.

Uh oh!

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

himself65 reacted with thumbs up emoji

All reactions

👍 1 reaction

himself65 Oct 16, 2023

Copy link

Copy Markdown

Member Author

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.

I will update doc later

Sorry, something went wrong.

Uh oh!

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

koshic reacted with eyes emoji

All reactions

👀 1 reaction

nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels

https://github.com/nodejs/node/actions/runs/6539860160

nodejs-github-bot commented Oct 16, 2023

Copy link

Copy Markdown

Collaborator

Commit Queue failed

- Loading data for nodejs/node/pull/50187
✔  Done loading data for nodejs/node/pull/50187
----------------------------------- PR info ------------------------------------
Title      stream: allow pass stream class to `stream.compose` (#50187)
Author     Alex Yang  (@himself65)
Branch     himself65:himself65/1014/spec -> nodejs:main
Labels     author ready, needs-ci
Commits    2
 - stream: allow pass stream class to `stream.compose`
 - fixup! fix: lint
Committers 1
 - Alex Yang 
PR-URL: https://github.com/nodejs/node/pull/50187
Fixes: https://github.com/nodejs/node/issues/50176
Reviewed-By: Moshe Atlow 
Reviewed-By: Robert Nagy 
Reviewed-By: Benjamin Gruenbaum 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50187
Fixes: https://github.com/nodejs/node/issues/50176
Reviewed-By: Moshe Atlow 
Reviewed-By: Robert Nagy 
Reviewed-By: Benjamin Gruenbaum 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 14 Oct 2023 22:05:57 GMT
   ✔  Approvals: 3
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/50187#pullrequestreview-1678677911
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/50187#pullrequestreview-1678678064
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/50187#pullrequestreview-1679839150
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-10-15T19:22:34Z: https://ci.nodejs.org/job/node-test-pull-request/54834/
- Querying data for job/node-test-pull-request/54834/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 50187
From https://github.com/nodejs/node
 * branch                  refs/pull/50187/merge -> FETCH_HEAD
✔  Fetched commits as 4032ad593c0d..83cf4248107a
--------------------------------------------------------------------------------
[main 843d25cb7c] stream: allow pass stream class to `stream.compose`
 Author: Alex Yang 
 Date: Sat Oct 14 17:04:43 2023 -0500
 2 files changed, 15 insertions(+)
[main 52a91a55af] fixup! fix: lint
 Author: Alex Yang 
 Date: Sat Oct 14 17:43:19 2023 -0500
 2 files changed, 2 insertions(+), 2 deletions(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

stream: allow pass stream class to stream.compose
PR-URL: #50187

Fixes: #50176

Reviewed-By: Moshe Atlow [email protected]

Reviewed-By: Robert Nagy [email protected]

Reviewed-By: Benjamin Gruenbaum [email protected]
[detached HEAD 92ba1de03d] stream: allow pass stream class to stream.compose

Author: Alex Yang [email protected]

Date: Sat Oct 14 17:04:43 2023 -0500

2 files changed, 15 insertions(+)

Rebasing (3/4)

Rebasing (4/4)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup! fix: lint
PR-URL: #50187

Fixes: #50176

Reviewed-By: Moshe Atlow [email protected]

Reviewed-By: Robert Nagy [email protected]

Reviewed-By: Benjamin Gruenbaum [email protected]
[detached HEAD 81e52e6e8e] fixup! fix: lint

Author: Alex Yang [email protected]

Date: Sat Oct 14 17:43:19 2023 -0500

2 files changed, 2 insertions(+), 2 deletions(-)
Successfully rebased and updated refs/heads/main.
ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

All reactions

Sorry, something went wrong.

Uh oh!

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

himself65 added a commit that referenced this pull request


          stream: allow pass stream class to stream.compose

ef7363e

PR-URL: #50187
Fixes: #50176
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

himself65 commented Oct 16, 2023

Copy link

Copy Markdown

Member Author

Landed in ef7363e

All reactions

Sorry, something went wrong.

Uh oh!

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

himself65 closed this

himself65 mentioned this pull request

docs: update api stream.compose #50206

Merged

targos pushed a commit that referenced this pull request


          stream: allow pass stream class to stream.compose

789372a

PR-URL: #50187
Fixes: #50176
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

targos commented Oct 23, 2023

Copy link

Copy Markdown

Member

Please think about assigning the semver-minor label when you add new features.

himself65 reacted with eyes emoji

All reactions

👀 1 reaction

Sorry, something went wrong.

Uh oh!

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

targos removed the commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. label

targos added the semver-minor PRs that contain new features and should be released in the next minor version. label

targos added a commit that referenced this pull request


          2023-10-24, Version 21.1.0 (Current)

5168feb

Notable changes:

doc:
  * add H4ad to collaborators (Vinícius Lourenço) #50217
esm:
  * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096
fs:
  * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095
lib:
  * (SEMVER-MINOR) add `navigator.userAgent` (Yagiz Nizipli) #50200
stream:
  * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187
  * call helper function from push and unshift (Raz Luvaton) #50173

PR-URL: #50335

targos mentioned this pull request