-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add batch
utility
#5
Conversation
c4b157e
to
03f5b17
Compare
I like this direction @RobinMalfait ! The As described in the readme update, this Then if we proceed to add more functions in this style, what I called |
src/eager.ts
Outdated
// TODO: Do we need to deal with this? | ||
if (isAsyncIterable(data) || data instanceof Promise) return data | ||
|
||
return { | ||
*[Symbol.iterator]() { | ||
const buffer = [] | ||
const iterator = data[Symbol.iterator]() | ||
let state = { more: true } | ||
|
||
while (state.more) { | ||
for (let i = 0; i < size; i++) { | ||
const { value, done } = iterator.next() | ||
if (done) state.more = false | ||
if (!done) buffer.push(value) | ||
} | ||
|
||
yield* buffer.splice(0) | ||
} |
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.
Regarding async iterables here, I think there's a good case to let them through and process them in the same manner!
On line 19
(when pulling out next()
), that would return a promise if data
is asynciterable, all that's needed to do in that case is await it, before extracting value
and done
. Everything else could stay the same, I think... await
on a non-promise would just resolve right away, am I right?
src/eager.test.ts
Outdated
const program = pipe( | ||
map(id => `/user/${id}`), | ||
map<string, unknown>(path => fetch(path)), | ||
eager(10), |
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.
If this test is processing 10 items, what about testing in batches of 5
or 4
, just to spice it up a bit? 😄
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.
Oh yes I agree. This allows me to implicitly test that it is doing the batching correctly!
+ add shortcut `wait` function, which is a bit more ergonomic
03f5b17
to
92895ff
Compare
knock knock is this thing still on? Welp, it took a while, but let's get this thing merged! |
No description provided.