Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: allow pass stream class to stream.compose #50187

Closed
wants to merge 2 commits into from

Conversation

himself65
Copy link
Member

@himself65 himself65 commented Oct 14, 2023

class A extends Transform {
  // ...
}

stream.compose(A) // old behavior: A will be called as a normal function and nothing will happen

Fixes: #50176

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 14, 2023
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2023
@nodejs-github-bot
Copy link
Collaborator

@himself65 himself65 added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2023
@nodejs-github-bot
Copy link
Collaborator

@MoLow MoLow added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 16, 2023
@@ -85,6 +85,11 @@ module.exports = function duplexify(body, name) {
if (typeof body === 'function') {
const { value, write, final, destroy } = fromAsyncGen(body);

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

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?

Copy link
Member Author

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

Copy link

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

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

Copy link
Member Author

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

Copy link

Choose a reason for hiding this comment

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

Stable pipeline also affected

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);
}

Copy link
Member Author

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

@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 16, 2023
@nodejs-github-bot
Copy link
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 moshe@atlow.co.il
Reviewed-By: Robert Nagy ronagy@icloud.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

[detached HEAD 92ba1de03d] stream: allow pass stream class to stream.compose
Author: Alex Yang himself65@outlook.com
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 moshe@atlow.co.il
Reviewed-By: Robert Nagy ronagy@icloud.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

[detached HEAD 81e52e6e8e] fixup! fix: lint
Author: Alex Yang himself65@outlook.com
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.

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

himself65 added a commit that referenced this pull request Oct 16, 2023
PR-URL: #50187
Fixes: #50176
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@himself65
Copy link
Member Author

Landed in ef7363e

@himself65 himself65 closed this Oct 16, 2023
targos pushed a commit that referenced this pull request Oct 23, 2023
PR-URL: #50187
Fixes: #50176
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos
Copy link
Member

targos commented Oct 23, 2023

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

@targos targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 23, 2023
@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 23, 2023
targos added a commit that referenced this pull request Oct 23, 2023
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 added a commit that referenced this pull request Oct 24, 2023
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 added a commit that referenced this pull request Oct 24, 2023
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 added a commit that referenced this pull request Oct 24, 2023
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 added a commit that referenced this pull request Oct 24, 2023
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
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#50187
Fixes: nodejs#50176
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
Notable changes:

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

PR-URL: nodejs#50335
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #50187
Fixes: #50176
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos added a commit that referenced this pull request Nov 12, 2023
Notable changes:

deps:
  * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908
doc:
  * add H4ad to collaborators (Vinícius Lourenço) #50217
esm:
  * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096
  * use import attributes instead of import assertions (Antoine du Hamel) #50140
  * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869
fs:
  * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095
  * add flush option to writeFile() functions (Colin Ihrig) #50009
lib:
  * (SEMVER-MINOR) add WebSocket client (Matthew Aitken) #49830
stream:
  * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187
  * call helper function from push and unshift (Raz Luvaton) #50173
  * optimize Writable (Robert Nagy) #50012
test_runner, cli:
  * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996
vm:
  * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141
  * use default HDO when importModuleDynamically is not set (Joyee Cheung) #49950
wasi:

PR-URL: #50682
targos added a commit that referenced this pull request Nov 12, 2023
Notable changes:

deps:
  * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908
doc:
  * add H4ad to collaborators (Vinícius Lourenço) #50217
esm:
  * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096
  * use import attributes instead of import assertions (Antoine du Hamel) #50140
  * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869
fs:
  * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095
  * add flush option to writeFile() functions (Colin Ihrig) #50009
lib:
  * (SEMVER-MINOR) add WebSocket client (Matthew Aitken) #49830
stream:
  * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187
  * call helper function from push and unshift (Raz Luvaton) #50173
  * optimize Writable (Robert Nagy) #50012
test_runner, cli:
  * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996
vm:
  * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141
  * use default HDO when importModuleDynamically is not set (Joyee Cheung) #49950
wasi:

PR-URL: #50682
targos added a commit that referenced this pull request Nov 21, 2023
Notable changes:

deps:
  * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908
doc:
  * add H4ad to collaborators (Vinícius Lourenço) #50217
esm:
  * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096
  * use import attributes instead of import assertions (Antoine du Hamel) #50140
  * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869
fs:
  * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095
  * add flush option to writeFile() functions (Colin Ihrig) #50009
lib:
  * (SEMVER-MINOR) add WebSocket client (Matthew Aitken) #49830
stream:
  * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187
  * call helper function from push and unshift (Raz Luvaton) #50173
  * optimize Writable (Robert Nagy) #50012
test_runner, cli:
  * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996
vm:
  * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141
  * use default HDO when importModuleDynamically is not set (Joyee Cheung) #49950
wasi:

PR-URL: #50682
targos added a commit that referenced this pull request Nov 22, 2023
Notable changes:

deps:
  * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908
doc:
  * add H4ad to collaborators (Vinícius Lourenço) #50217
esm:
  * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096
  * use import attributes instead of import assertions (Antoine du Hamel) #50140
  * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869
fs:
  * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095
  * add flush option to writeFile() functions (Colin Ihrig) #50009
lib:
  * (SEMVER-MINOR) add WebSocket client (Matthew Aitken) #49830
stream:
  * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187
  * call helper function from push and unshift (Raz Luvaton) #50173
  * optimize Writable (Robert Nagy) #50012
test_runner, cli:
  * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996
vm:
  * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141
  * use default HDO when importModuleDynamically is not set (Joyee Cheung) #49950
wasi:

PR-URL: #50682
martenrichter pushed a commit to martenrichter/node that referenced this pull request Nov 26, 2023
Notable changes:

deps:
  * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) nodejs#49908
doc:
  * add H4ad to collaborators (Vinícius Lourenço) nodejs#50217
esm:
  * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) nodejs#50096
  * use import attributes instead of import assertions (Antoine du Hamel) nodejs#50140
  * --experimental-default-type flag to flip module defaults (Geoffrey Booth) nodejs#49869
fs:
  * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) nodejs#50095
  * add flush option to writeFile() functions (Colin Ihrig) nodejs#50009
lib:
  * (SEMVER-MINOR) add WebSocket client (Matthew Aitken) nodejs#49830
stream:
  * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) nodejs#50187
  * call helper function from push and unshift (Raz Luvaton) nodejs#50173
  * optimize Writable (Robert Nagy) nodejs#50012
test_runner, cli:
  * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) nodejs#49996
vm:
  * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) nodejs#50141
  * use default HDO when importModuleDynamically is not set (Joyee Cheung) nodejs#49950
wasi:

PR-URL: nodejs#50682
lucshi pushed a commit to lucshi/node that referenced this pull request Nov 27, 2023
Notable changes:

deps:
  * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) nodejs#49908
doc:
  * add H4ad to collaborators (Vinícius Lourenço) nodejs#50217
esm:
  * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) nodejs#50096
  * use import attributes instead of import assertions (Antoine du Hamel) nodejs#50140
  * --experimental-default-type flag to flip module defaults (Geoffrey Booth) nodejs#49869
fs:
  * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) nodejs#50095
  * add flush option to writeFile() functions (Colin Ihrig) nodejs#50009
lib:
  * (SEMVER-MINOR) add WebSocket client (Matthew Aitken) nodejs#49830
stream:
  * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) nodejs#50187
  * call helper function from push and unshift (Raz Luvaton) nodejs#50173
  * optimize Writable (Robert Nagy) nodejs#50012
test_runner, cli:
  * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) nodejs#49996
vm:
  * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) nodejs#50141
  * use default HDO when importModuleDynamically is not set (Joyee Cheung) nodejs#49950
wasi:

PR-URL: nodejs#50682
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

impossible to use spec test reporter without new
8 participants