node icon indicating copy to clipboard operation
node copied to clipboard

Allow manipulating the generator in Duplex.from()

Open matthieusieben opened this issue 1 year ago • 10 comments

Fix for #55077

matthieusieben avatar Sep 24 '24 09:09 matthieusieben

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

nodejs-github-bot avatar Sep 24 '24 11:09 nodejs-github-bot

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.00%. Comparing base (4efb7ae) to head (d660809). Report is 122 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #55096   +/-   ##
=======================================
  Coverage   87.99%   88.00%           
=======================================
  Files         656      656           
  Lines      188988   189033   +45     
  Branches    35988    35995    +7     
=======================================
+ Hits       166302   166350   +48     
+ Misses      15848    15844    -4     
- Partials     6838     6839    +1     
Files with missing lines Coverage Δ
lib/internal/streams/duplexify.js 97.16% <100.00%> (+0.59%) :arrow_up:

... and 29 files with indirect coverage changes

codecov[bot] avatar Sep 24 '24 14:09 codecov[bot]

~Not sure how to fix the breaking test(s):~

test-stream-compose-operator
=== release test-stream-compose-operator ===
Path: parallel/test-stream-compose-operator
node:internal/process/promises:394
    triggerUncaughtException(err, true /* fromPromise */);
    ^

AssertionError [ERR_ASSERTION]: The input did not match the regular expression /boom/. Input:

'AbortError: The operation was aborted'

    at process.processTicksAndRejections (node:internal/process/task_queues:105:5) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: AbortError: The operation was aborted
      at destroyer (node:internal/streams/destroy:328:11)
      at node:internal/streams/duplexify:89:7
      at asyncGenerator.return (node:internal/streams/duplexify:245:9)
      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
      at async /Users/msi/Github/nodejs/node/test/parallel/test-stream-compose-operator.js:63:5
      at async nextAsync (node:internal/streams/from:182:33) {
    code: 'ABORT_ERR'
  },
  expected: /boom/,
  operator: 'rejects'
}

Node.js v23.0.0-pre
Command: out/Release/node /Users/msi/Github/nodejs/node/test/parallel/test-stream-compose-operator.js

Fixed thanks to @jazelly 🙏

matthieusieben avatar Sep 24 '24 19:09 matthieusieben

@benjamingr This seems super hacky... wdyt?

ronag avatar Sep 25 '24 05:09 ronag

@ronag I agree. I don't usually like to monkey patch stuff but in this case I coudn't think of another way...

I tried using a try/catch/finally block inside the async generator to do the same. But since those block don't get called unless an iteration started (next() called at least once), that didn't work.

matthieusieben avatar Sep 26 '24 11:09 matthieusieben

This PR needs a rebase

jazelly avatar Nov 23 '24 08:11 jazelly

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

nodejs-github-bot avatar Nov 29 '24 10:11 nodejs-github-bot

Just rebased and fixed conflicts.

matthieusieben avatar Nov 29 '24 11:11 matthieusieben

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

nodejs-github-bot avatar Nov 30 '24 03:11 nodejs-github-bot

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

nodejs-github-bot avatar Nov 30 '24 10:11 nodejs-github-bot

Commit Queue failed
- Loading data for nodejs/node/pull/55096
✔  Done loading data for nodejs/node/pull/55096
----------------------------------- PR info ------------------------------------
Title      Allow manipulating the generator in Duplex.from() (#55096)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     matthieusieben:fix-55077 -> nodejs:main
Labels     stream, semver-minor, test, needs-ci
Commits    3
 - stream: handle generator destruction from Duplex.from()
 - stream: add tests for Duplex.from()
 - stream: properly destroy duplexified functions
Committers 1
 - Matthieu Sieben <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/55096
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Jason Zhang <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55096
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Jason Zhang <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 24 Sep 2024 09:05:21 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/55096#pullrequestreview-2496213343
   ✔  - Jason Zhang (@jazelly): https://github.com/nodejs/node/pull/55096#pullrequestreview-2470700576
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-11-30T10:16:00Z: https://ci.nodejs.org/job/node-test-pull-request/63811/
- Querying data for job/node-test-pull-request/63811/
   ✔  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 55096
From https://github.com/nodejs/node
 * branch                  refs/pull/55096/merge -> FETCH_HEAD
✔  Fetched commits as a1d980c4e058..d66080905684
--------------------------------------------------------------------------------
[main 039174a4d9] stream: handle generator destruction from Duplex.from()
 Author: Matthieu Sieben <[email protected]>
 Date: Tue Sep 24 11:04:07 2024 +0200
 2 files changed, 189 insertions(+), 7 deletions(-)
[main 027294b9d9] stream: add tests for Duplex.from()
 Author: Matthieu Sieben <[email protected]>
 Date: Tue Sep 24 21:31:23 2024 +0200
 2 files changed, 57 insertions(+), 3 deletions(-)
[main c7e870905c] stream: properly destroy duplexified functions
 Author: Matthieu Sieben <[email protected]>
 Date: Thu Sep 26 13:25:26 2024 +0200
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
Rebasing (2/6)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
stream: handle generator destruction from Duplex.from()

PR-URL: https://github.com/nodejs/node/pull/55096 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Jason Zhang <[email protected]>

[detached HEAD fd5069920c] stream: handle generator destruction from Duplex.from() Author: Matthieu Sieben <[email protected]> Date: Tue Sep 24 11:04:07 2024 +0200 2 files changed, 189 insertions(+), 7 deletions(-) Rebasing (3/6) Rebasing (4/6) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- stream: add tests for Duplex.from()

PR-URL: https://github.com/nodejs/node/pull/55096 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Jason Zhang <[email protected]>

[detached HEAD e748795b16] stream: add tests for Duplex.from() Author: Matthieu Sieben <[email protected]> Date: Tue Sep 24 21:31:23 2024 +0200 2 files changed, 57 insertions(+), 3 deletions(-) Rebasing (5/6) Rebasing (6/6) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- stream: properly destroy duplexified functions

Co-authored-by: Jason Zhang <[email protected]> PR-URL: https://github.com/nodejs/node/pull/55096 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Jason Zhang <[email protected]>

[detached HEAD 3a4c553d64] stream: properly destroy duplexified functions Author: Matthieu Sieben <[email protected]> Date: Thu Sep 26 13:25:26 2024 +0200 1 file changed, 1 insertion(+), 1 deletion(-) 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/12280911806

nodejs-github-bot avatar Dec 11 '24 16:12 nodejs-github-bot

Hello maintainers,

I am not sure what commit-queue-failed means or how to fix it. Also, it appears that an approval from @ronag is still needed.

Would love to see this land 🙏

matthieusieben avatar Dec 12 '24 08:12 matthieusieben

Landed in 55413004c8ab489a03793b80c20d2ec6552668c0

nodejs-github-bot avatar Dec 12 '24 08:12 nodejs-github-bot