Why is `forEach` discouraged?

[zeke]

I am using this linter in a project and it raised an issue about code like this:

apples
  .filter(apple => !apple.bad)
  .forEach(doSomething)

A for loop would have been messy in this context as the forEach had a number of const declarations. I ended up working around it by changing the forEach to map.

What is the motivation for this rule? My guess is that forEach doesn't play nicely with async functions, but I'm not sure. The current doc doesn't provide any explanation of the rationale: https://github.com/github/eslint-plugin-github/blob/master/docs/rules/array-foreach.md

cc @github/product-docs-engineering
cc @josh @koddsson @keithamus

[koddsson]

This rule was added before my time at GitHub but I think was added to steer developers away from converting NodeList to an array just to be able to call forEach on it.

// Doesn't work since `document.querySelectorAll` returns a NodeList and not an Array.
document.querySelectorAll('.js-some-element').forEach(..)

// Works since we've converted the NodeList to an Array.
Array.from(document.querySelectorAll('.js-some-element')).forEach(..)

Since we work a lot with the .querySelectorAll method on github.com we operate a lot on NodeList and showing developers how to iterate over it rather than converting it to an array first is probably the intent here.

It has the drawback of giving you false positives when working with non NodeList variables though like you've experienced.

[ldeluigi]

Please fix this rule: for .. of can't be used to iterate with index.