-
Notifications
You must be signed in to change notification settings - Fork 30k
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
doc: update stream.reduce
concurrency note
#47166
doc: update stream.reduce
concurrency note
#47166
Conversation
2a31082
to
4b92bfd
Compare
2ac4139
to
487f5db
Compare
487f5db
to
9627c88
Compare
```mjs | ||
import { Readable } from 'node:stream'; | ||
import { readdir, stat } from 'node:fs/promises'; | ||
import { join } from 'node:path'; | ||
|
||
const ten = await Readable.from([1, 2, 3, 4]).reduce((previous, data) => { | ||
return previous + data; | ||
}); | ||
console.log(ten); // 10 | ||
const directoryPath = './src'; | ||
const filesInDir = await readdir(directoryPath); | ||
|
||
const folderSize = await Readable.from(filesInDir) | ||
.reduce(async (totalSize, file) => { | ||
const { size } = await stat(join(directoryPath, file)); | ||
return totalSize + size; | ||
}, 0); | ||
|
||
console.log(folderSize); | ||
``` |
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.
Updated this example as well so the refactor can be more obvious
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.
lgtm
Hey @VoltrexKeyva , can you please rereview so it can be merged? |
Thanks @VoltrexKeyva , can you merge it? |
Landed in c588145 |
PR-URL: #47166 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
PR-URL: #47166 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
PR-URL: #47166 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
PR-URL: #47166 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
At first, when I read that note I thought I should do this:
This did not work and does not make any sense as the map items are emitted in the order that got from the readable stream.
So I thought of changing to note to recommend implementing reduce using
.forEach
with concurrency but then I understand what @benjamingr meant when he wrote that note - because the only way you will need concurrency is if you do some async operation (because sync block the loop - bla bla) and that operation can be parallelized so I updated the note with an example@benjamingr, hope this is what you meant 😅