stream: add reduce #41669
stream: add reduce #41669
Conversation
|
Review requested: |
benjamingr
force-pushed
the
stream-iterator-helper-reduce
branch
from
0be4340
to
ea5d783
Compare
doc/api/stream.md
Outdated
| @@ -2118,6 +2118,45 @@ import { Readable } from 'stream'; | |||
| await Readable.from([1, 2, 3, 4]).take(2).toArray(); // [1, 2] | |||
| ``` | |||
|
|
|||
| ### `readable.reduce(fn[, initial[, options])` | |||
There was a problem hiding this comment.
| ### `readable.reduce(fn[, initial[, options])` | |
| ### `readable.reduce(fn[, initial[, options]])` |
To align with what the code currently supports. What if you want to reduce() with options but without an initial value?
There was a problem hiding this comment.
How would we differentiate an initial value not existing? An object containing a signal can be a valid initialValue for reduce
There was a problem hiding this comment.
An ugly solution would be reduce(fn, undefined, options) and changing the spec from "initialValue is not present" to "initialValue is not undefined".
There was a problem hiding this comment.
I think that would also be a hard sell because it diverges from how Array.prototype.reduce behaves:
[1,2,3].reduce((p, c) => p + c); // 6
[1,2,3].reduce((p, c) => p + c, undefined); // NaN
There was a problem hiding this comment.
Whatever is decided, there is something to fix here because one of the opening brackets isn't closed.
There was a problem hiding this comment.
Ah, didn't realize this was blocking the review, fixed.
benjamingr
force-pushed
the
stream-iterator-helper-reduce
branch
2 times, most recently
from
fb751fa
to
aafca2a
Compare
benjamingr
force-pushed
the
stream-iterator-helper-reduce
branch
2 times, most recently
from
cb6672d
to
9976118
Compare
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Commit Queue failed- Loading data for nodejs/node/pull/41669 ✔ Done loading data for nodejs/node/pull/41669 ----------------------------------- PR info ------------------------------------ Title stream: add reduce (#41669) Author Benjamin Gruenbaum (@benjamingr) Branch benjamingr:stream-iterator-helper-reduce -> nodejs:master Labels stream, needs-ci, commit-queue-squash Commits 3 - stream: add reduce - fixup! bad markup - fixup! Update lib/internal/streams/operators.js Committers 2 - Benjamin Gruenbaum - GitHub PR-URL: https://github.com/nodejs/node/pull/41669 Reviewed-By: Matteo Collina ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/41669 Reviewed-By: Matteo Collina -------------------------------------------------------------------------------- ℹ This PR was created on Sun, 23 Jan 2022 13:57:13 GMT ✔ Approvals: 1 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/41669#pullrequestreview-867069404 ✖ This PR needs to wait 2 more hours to land (or 0 hours if there is one more approval) ✖ GitHub CI is still running ℹ Last Full PR CI on 2022-01-30T10:55:05Z: https://ci.nodejs.org/job/node-test-pull-request/42234/ - Querying data for job/node-test-pull-request/42234/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/1768552118 |
|
ok let's wait for the GitHub ci |
|
Landed in d2ac192 |
|
this looks like a Also should it be a |
PR-URL: #41669 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This continues the work in #41630 so that one has to land before this one and only look at the last commit :) (same as last time, I want feedback :))I have a few questions here about what the behaviour should be and I think my implementation may be simplified (I changed it as I added tests).
cc @ronag @mcollina @aduh95 @vweevers