stream: add flatMap to readable

[benjamingr]

דארןלק<The normal caveat of this shouldn't landing until #41573 does and being a draft until then and to only look at the relevant commit :)

I am thinking about changing the toArray() behavior to not flatten a non-object-mode stream since I think things get tricky when flatMaping binary and non-binary streams together but I am honestly not sure and want feedback on much of the behaviours here.

Like the other iterator helpers PRs, this involves work co-authored by @ronag

cc @nodejs/streams

I am mostly interested in feedback about what the semantics should be and how flatMap works when flatMapping one stream mode and returning the other type from the mapper.

[nodejs-github-bot]

Review requested:

• @nodejs/streams

[MadaraUchiha]

What if options.signal is already aborted?

[benjamingr]

Not related to this PR move it to the some/every PR?

[ronag]

I think toArray should match flatMap

[benjamingr]

@ronag I was about to ask about the interop between objectMode and non objectMode streams. Then noticed that all operators call Readable.from which by default returns objectMode stream. Is that intentional? Assuming it is maybe we should remove the non-objectMode behaviour from toArray?

[ronag]

It's intentional. I think we should disregard non objectMode in these cases.

[benjamingr]

It's intentional. I think we should disregard non objectMode in these cases.

Ok, so should I change the behaviour of toArray to disregard objectMode? It can also solve the Uint8Array/Buffer debate there :)

[benjamingr]

Rebased, added a few tests. This should be ready for review @nodejs/streams

[mcollina]

lgtm

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[benjamingr]

Seems to fail (only on arm):

node:internal/process/promises:265
            triggerUncaughtException(err, true /* fromPromise */);
            ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  Comparison {
+   name: 'TypeError'
-   name: 'AbortError'
  }
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at runNextTicks (node:internal/process/task_queues:65:3)
    at listOnTimeout (node:internal/timers:528:9)
    at processTimers (node:internal/timers:502:7) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: TypeError: Cannot read properties of undefined (reading 'Symbol(Symbol.asyncIterator)')
      at Readable.flatMap (node:internal/streams/operators:235:5)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at runNextTicks (node:internal/process/task_queues:65:3)
      at listOnTimeout (node:internal/timers:528:9)
      at processTimers (node:internal/timers:502:7)
      at async next (node:internal/streams/from:85:11),
  expected: { name: 'AbortError' },
  operator: 'rejects'
}

[benjamingr]

This seems to be timing related where setImmediate runs after the await setTimeout(100) finishes. I'm wondering if this possibility (setImmediate running after setTimeout(100)) is known in arm @nodejs/timers

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/41612
✔  Done loading data for nodejs/node/pull/41612
----------------------------------- PR info ------------------------------------
Title      stream: add flatMap to readable (#41612)
Author     Benjamin Gruenbaum  (@benjamingr)
Branch     benjamingr:stream-iterator-helper-flatMap -> nodejs:master
Labels     stream, semver-minor, needs-ci
Commits    1
 - stream: support flatMap
Committers 1
 - Benjamin Gruenbaum 
PR-URL: https://github.com/nodejs/node/pull/41612
Reviewed-By: Robert Nagy 
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41612
Reviewed-By: Robert Nagy 
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - stream: support flatMap
   ℹ  This PR was created on Thu, 20 Jan 2022 12:04:31 GMT
   ✔  Approvals: 2
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/41612#pullrequestreview-858215585
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/41612#pullrequestreview-859599984
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-01-22T22:05:54Z: https://ci.nodejs.org/job/node-test-pull-request/42092/
- Querying data for job/node-test-pull-request/42092/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

[benjamingr]

I'll land manually and assume robert and matteo are fine with the test fixes for exotic platforms landing.

[benjamingr]

Landed in cc8931a 🎉