Fix foreach

[devotox]

@toniov @robjtede @GrayedFox @N0ps32

[robjtede]

Seems reasonable except using Promise as the callback return type actually has stricter requirements for the callbacks. See this playground for the effect of this change.

TL;DR The current return type is fine. If anything, this PR is harmful to DX.

[salolivares]

Agree with @robjtede. However on newer versions of ESLint (w/typescript) it throws an error/warning:

Promise returned in function argument where a void return was expected  @typescript-eslint/no-misused-promises

with something like:

import { forEach } from 'p-iteration';

await forEach(section.content, async (content) => {
    if (content.data) {
    ....

Wish there is a way we can type it better -- for now, I'm just disabling the ESLint rule.

[toniov]

Sorry for the late reaction, I have been out for a while.

I'm not too familiar with Typescript, but to me both ways make sense. If I have to choose I prefer to keep the flexibility we have now.

Feel free to comment here if you have a better idea or good arguments about this, so I can reopen the PR.

Thank you everyone!

[devotox]

The reason for the change is that you are looking for void but it is also a Promise and thus we get this issue

Promise returned in function argument where a void return was expected  @typescript-eslint/no-misused-promises

[devotox]

@toniov @salolivares @robjtede @GrayedFox what if we did this with void | Promise<void> ?

[GrayedFox]

Hmm, for a second I thought you meant || in order to short circuit the return type (haha).

I realize now you want to extend the function definition return types to include both void and Promise<void>.

The thing is by specifying void as the return type the code already technically allows and includes a Promise<void> as a possible returned type (the difference being that in the latter case, a Promise is returned that resolves to undefined as opposed to evaluating the expression and then returning undefined directly). Specifying void as the returned type does not preclude Promise<void> as once such possibility.

By it's very nature the void operator "evaluates the given expression and then returns undefined", which is in my understanding what we want here: the forEach and forEachSeries methods in the p-iteration module are designed to allow developers to use Promises (or the await keyword) within these prototype array methods which themselves don't return a type.

Think about how we use the standard forEach method on an array i.e. open up your console and run something like this:

let myVar = 'suchVar';

[1, 2, 3, 4].forEach(el => console.log(el))
// logs 1, 2, 3, 4, normal usage of forEach

console.log(myVar);
// logs 'suchVar'

myVar = [1, 2, 3, 4].forEach(el => console.log(el))
// logs 1, 2, 3, 4, abnormal usage of forEach (this is a code smell)

console.log(myVar)
// logs undefined, since the returned value of forEach is always undefined

I could also wrap up the returned type of forEach into a Promise if I really wanted to: this still won't change the fact that forEach returns undefined and that is also what we want the p-iteration module to do. I haven't really used typescript that much but the docs seem to encourage using void for "a function... that doesn't have a value".

Long story short: I think this is one of the few cases where the no-misused-promises rule is overreaching.

That or should we instead do the following (untested):

  export const forEachSeries: <T>(
    array: T[],
    callback: (currentValue: T, index: number, array: T[]) => void,
    thisArg?: any
  ) => void;

Does the above get rid of the lint warning and not break anything?

[GrayedFox]

This stack overflow answer makes the difference between returning void and Promise<void> explicit and is pretty succinct.