-
Notifications
You must be signed in to change notification settings - Fork 74
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
feat: add concurrentPool
#306
Conversation
53cbcf3
to
b19eece
Compare
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.
I think the concurrentPool is same here code logic that written by @indongyoo. Referencing this code might be helpful.
src/Lazy/concurrentPool.ts
Outdated
await chainItem(item); | ||
} catch (error) { | ||
idleWorkers++; | ||
finished = true; | ||
|
||
const item = { | ||
success: { done: false }, | ||
fail: error, | ||
} as Item<A>; | ||
|
||
itemMap.set(id, item); | ||
await chainItem(item); | ||
} | ||
} | ||
|
||
async function chainItem(item: Item<A>) { | ||
prev = prev.then(() => item).then(pullItem); | ||
return item; | ||
} |
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.
Is there any reason to use the async
keyword if await is not used inside chainItem?
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.
I didn't remove what I had previously declared. Thank you.
* @experimental concurrentPool | ||
*/ | ||
function concurrentPool<A>( | ||
length: number, |
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.
I’m not sure if the name length clearly represents its role. Is this a common variable name in functions dealing with concurrency?
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.
I tried to match it with the variable name of concurrent.
When changing it, it would be better to proceed with the variable name of concurrent!
ex) concurrency
?
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.
got it
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
* map((a) => delay(100 * a, a)), | ||
* concurrentPool(3), | ||
* each(console.log), // log 1, 2, 3, 4, 5, 6 | ||
* ); // 2 seconds |
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.
missing backticks
https://fxts.dev/docs/concurrentPool
@ppeeou
Fixes #304
I'd like to get feedback from you @ysm-dev
What do you think about the expression and behavior?