Use for of instead of forEach where possible

[dapplion]

for of instead of forEach:

• It works on any iterable
• It supports all kinds of control flow in the loop body, like continue, break, return, yield and await.
• on errors there's one less stack line
• forEach can potentially hide problems such as unhandled promises
• personally, I find it more readable (tho this is subjective)

[mpetrunic]

TBH I like forEach more:

• it's easier to cast type of item being iterated
• easier access to item index and update of array being iterated
• I frequently confuse for-in and for-of (having hard time figuring what iterates over what)

[dapplion]

TBH I like forEach more:

• it's easier to cast type of item being iterated
• easier access to item index and update of array being iterated
• I frequently confuse for-in and for-of (having hard time figuring what iterates over what)

.forEach is great for the scenarios that you described. This PR only changes the cases where none of these conditions are met. Also for ... in should never be used, but use for ... of Object.keys()

[dapplion]

By using for of I found to instances of not handleded promises. Tagging the previous author according to git blame

1. @tuyennhv could you take a look at this one
   

   lodestar/packages/lodestar/src/sync/regular/oneRangeAhead/processor.ts

   Line 51 in 8059f34

   this.chain.receiveBlock(block);

   
   Before it was

sortedBlocks.forEach((block) => this.chain.receiveBlock(block, false));

Should it be awaited or not?

2. @mpetrunic could you take a look at this one
   

   lodestar/packages/lodestar/test/e2e/sync/sync.test.ts

   Line 31 in 8059f34

   validator.start();

validators.forEach((v) => v.start());

[mpetrunic]

1. @mpetrunic could you take a look at this one
   

   lodestar/packages/lodestar/test/e2e/sync/sync.test.ts

   Line 31 in 8059f34

   validator.start();

validators.forEach((v) => v.start());

We don't really care about awaiting there. If we are gonna await, Promise.all should be better fit.

That said, it has nothing to do with for-of vs forEach. It's just that in forEach callback we didn't put async keyword (validators.forEach(async (v) => {v.start()});). And once you moved it into async scope it shows error. It wouldn't display error if for-of is executed in non-async context/method.

[dapplion]

It wouldn't display error if for-of is executed in non-async context/method.

The error @typescript-eslint/no-floating-promises still shows in non-async context, you can try it

[codeclimate]

Code Climate has analyzed commit 5597e5c and detected 6 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	6

View more on Code Climate.

[mpetrunic]

It wouldn't display error if for-of is executed in non-async context/method.

The error @typescript-eslint/no-floating-promises still shows in non-async context, you can try it

add brackets in forEach :)

[dapplion]

add brackets in forEach :)

Sure, but here we should compare the one-line syntax to be fair.

validators.forEach(v => v.start())

for (const v of validators) v.start()

The error will show up in the second and not in the first. In multiline, it will show up in both styles

[mpetrunic]

add brackets in forEach :)

Sure, but here we should compare the one-line syntax to be fair.

validators.forEach(v => v.start())

for (const v of validators) v.start()

The error will show up in the second and not in the first. In multiline, it will show up in both styles

That's not equivalent.
Without brackets there is implicit return so in for-of that would be:

for (const v of validators) {
   (function() { return return v.start() })()
}

[dapplion]

add brackets in forEach :)

Sure, but here we should compare the one-line syntax to be fair.

validators.forEach(v => v.start())

for (const v of validators) v.start()

The error will show up in the second and not in the first. In multiline, it will show up in both styles

That's not equivalent.
Without brackets there is implicit return so in for-of that would be:

for (const v of validators) {
   (function() { return return v.start() })()
}

Why would you want that in a for loop?

[mpetrunic]

Why would you want that in a for loop?

You don't. You don't wan't that in forEach which is what we/I did there since void-ing promise wasn't working in previous eslint version when you didn't want to await promise.

[twoeths]

By using for of I found to instances of not handleded promises. Tagging the previous author according to git blame

1. @tuyennhv could you take a look at this one
   

   lodestar/packages/lodestar/src/sync/regular/oneRangeAhead/processor.ts

   Line 51 in 8059f34

   this.chain.receiveBlock(block);

   Before it was

sortedBlocks.forEach((block) => this.chain.receiveBlock(block, false));

Should it be awaited or not?

no we don't have to but it should also be ok to use await here too

[dapplion]

no we don't have to but it should also be ok to use await here too

Would you mind taking a look yourself @tuyennhv? That function uses non straightforward Promise pattern that I don't understand

[wemeetagain]

Can you see if there is a noticeable performance cost to using for-of instead of forEach?

[dapplion]

Can you see if there is a noticeable performance cost to using for-of instead of forEach?

This is a very popular question and different benchmarks show different results, where sometimes forEach wins others for ... of. I guess it depends on the platform or engine then? If performance is important somewhere (like SSZ bytes comparison), we should use the regular for loop which is faster and noticeable once you do very large numbers of iterations (+1e6)

[bajohn]

Can you see if there is a noticeable performance cost to using for-of instead of forEach?

This is a very popular question and different benchmarks show different results, where sometimes forEach wins others for ... of. I guess it depends on the platform or engine then? If performance is important somewhere (like SSZ bytes comparison), we should use the regular for loop which is faster and noticeable once you do very large numbers of iterations (+1e6)

I think the difference between the two methods would be insignificant compared to the async operation happening inside the loop, probably not the most important place to optimize!

[dapplion]

I think the difference between the two methods would be insignificant compared to the async operation happening inside the loop, probably not the most important place to optimize!

Agree, the reasons for choosing between for of and forEach should be code readability, features and subjective preference