-
Notifications
You must be signed in to change notification settings - Fork 1
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
KRNL-221 add callChunked array function #41
Conversation
processFn: (arrayChunk: T[]) => Promise<void>, | ||
): Promise<void> { | ||
for (let i = 0; i < array.length; i += chunkSize) { | ||
const arrayChunk = array.slice(i, i + chunkSize) |
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.
🔶 Very surprised that you don't use the chunk
function directly above. If you did you could have:
const chunks = chunk(array, chunkSize)
for (const arrayChunk of chunks) {
await processFn(arrayChunk)
}
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.
This can also be done as a promise reducer:
chunk(array, chunkSize)
.reduce(
(promiseChain, chunk) => promiseChain.then(() => callback(chunk)),
Promise.resolve()
)
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.
for of
is slower than old-school looping, so for a low-level library I would recommend keeping the original approach.
see https://github.com/kibertoad/nodejs-benchmark-tournament/blob/master/loops/_results/results.md
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.
And I'm not sure how I feel about using reduce here, sounds like an overcomplication. Original version is pretty simple.
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.
So, what should I do at the end?:)
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.
Personally I see no reason to change this code, for me it is as simple as it needs to be, I see no advantages, performance or complexity-wise in suggested alternatives.
@rupert-mckay This is a yellow, non-blocking-but-please-explain-your-position comment, right?
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.
Yup. Happy for the PR to merge as-is. Still a bit surprised that the chunk
helper isn't used, but it's not important.
@@ -14,3 +14,14 @@ export function chunk<T>(array: T[], chunkSize: number): T[][] { | |||
// eslint-disable-next-line @typescript-eslint/no-unsafe-return |
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.
🟢 Feel free to ignore this comment as beyond the scope of this PR... but I really don't like what's going on in the chunk
function. Ignoring no-unsafe-return
is dangerous, and the implementation is especially fiddly and prone to error.
Chunking can be expressed rather naturally with a generator function which yields one chunk at a time. We can gather them into an array with Array.from
. Here's an example alternative implementation:
function* chunkGenerator<Item>(
array: readonly Item[],
chunkSize = 1,
) {
let index = 0;
while (index < array.length) {
yield array.slice(index, index + chunkSize);
index += chunkSize;
}
}
const chunk = <Item>(
array: readonly Item[],
chunkSize = 1,
) => Array.from(chunkGenerator(array, chunkSize));
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.
Feel free to ignore this comment
This seems to contradict the RED flag.
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.
That's fair. My instinct had been "this is something I feel very strongly about"... but it is naturally contradicted by "... but it might not at all be relevant to this PR".
I'll edit to 🟢
Changes
Add callChunked function
Checklist
major
,minor
,patch
orskip-release