Skip to content
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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Mar 23, 2023
Merged

KRNL-221 add callChunked array function #41

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b9e7cc0
KRNL-221 add callChunked array function
Dosexe Mar 22, 2023
1a4d339
KRNL-221 add callChunked documentation
Dosexe Mar 22, 2023
b89c83d
KRNL-221 fix callChunked signature
Dosexe Mar 22, 2023
64f879a
KRNL-221 fix tests
Dosexe Mar 22, 2023
aaedab9
KRNL-221 fix array signature
Dosexe Mar 22, 2023
e526406
KRNL-221 style fixes
Dosexe Mar 23, 2023
044553c
KRNL-221 style fixes
Dosexe Mar 23, 2023
58ddc2d
KRNL-221 style fixes
Dosexe Mar 23, 2023
ce1ff5a
KRNL-221 style fixes
Dosexe Mar 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions docs/array-utils.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,13 @@

Splits `array` into an array of arrays, each sub-array being no larger than the provided `chunkSize`,
preserving original order of the elements.

```typescript
async function callChunked<T>(
chunkSize: number,
array: readonly T[],
processFn: (arrayChunk: T[]) => Promise<void>,
): Promise<void>
```

Splits `array` by passed `chunkSize` and run callback asynchronously for every chunk in a sequential order.
10 changes: 9 additions & 1 deletion src/utils/arrayUtils.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { chunk } from './arrayUtils'
import { callChunked, chunk } from './arrayUtils'

describe('chunk', () => {
it('empty array returns empty array', () => {
Expand Down Expand Up @@ -33,4 +33,12 @@ describe('chunk', () => {
[6, 7],
])
})
it('should call function with chunked array', async () => {
const array = [1, 2, 3, 4, 5]
Dosexe marked this conversation as resolved.
Show resolved Hide resolved
const myMock = jest.fn()
myMock.mockReturnValueOnce([1, 2]).mockReturnValueOnce([3, 4]).mockReturnValue([5])
await callChunked(2, array, async (arrayChunk) => {
expect(arrayChunk).toStrictEqual(await myMock())
})
})
})
11 changes: 11 additions & 0 deletions src/utils/arrayUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,14 @@ export function chunk<T>(array: T[], chunkSize: number): T[][] {
// eslint-disable-next-line @typescript-eslint/no-unsafe-return
Copy link
Contributor

@rupert-mckay rupert-mckay Mar 22, 2023 •
edited
Loading

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));

Copy link
Collaborator

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.

Copy link
Contributor

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 🟢

return result
}

export async function callChunked<T>(
chunkSize: number,
array: readonly T[],
processFn: (arrayChunk: T[]) => Promise<void>,
Dosexe marked this conversation as resolved.
Show resolved Hide resolved
): Promise<void> {
for (let i = 0; i < array.length; i += chunkSize) {
const arrayChunk = array.slice(i, i + chunkSize)
Copy link
Contributor

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)
}

Copy link
Contributor

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()
  )

Copy link
Collaborator

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

Copy link
Collaborator

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.

Copy link
Contributor Author

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?:)

Copy link
Collaborator

@kibertoad kibertoad Mar 23, 2023 •
edited
Loading

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?

Copy link
Contributor

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.

await processFn(arrayChunk)
}
}