noForEach shouldn't be default on

[kurtextrem]

Hi,
noForEach mentions this:

Performance: Using forEach can lead to performance issues, especially when working with large arrays. When more requirements are added on, forEach typically gets chained with other methods like filter or map, causing multiple iterations over the same Array. Encouraging for loops discourages chaining and encourages single-iteration logic (e.g. using a continue instead of filter).

While the latter makes sense, the former doesn't. I've benchmarked forEach vs. for..of using mitata in the latest nodejs (v22), as well as in d8 (v8's debug shell) on the latest version (from source) and bench-node to have a third benchmark source.

Here's the result in d8:

Latest node:

bench-node:

Source: https://github.com/kurtextrem/foreach-bench

So from the benchmarks (m2 Pro laptop), forEach does not seem to be meaningfully slower with large arrays (bench-node result, so guaranteed to be unoptimized), and with mitatas result, forEach even seems slightly faster.

[ematipico]

I think the documentation shouldn't talk about performance for two reasons:

• what you just demonstrated! Thank you very much for taking the time to run these benchmarks
• performance depends on the runtime, and the runtime, with time, is able to improve the performance of certain functions/features

If we removed the "speed" part from the documentation, would you be willing to keep it as recommended?

[dev-nicolaos]

I'd agree that this rule should not be in the recommended set, at least in its current form.

In addition to the questionable performance claims addressed above, the justification for the rule cites the improved readability of for...of. That may be true in some cases but certainly not all. The rule currently applies to any object with a forEach method, including Maps. Take a look at this example.

const myMap = new Map();

// calls to myMap.set()

myMap.forEach((item) => item.method());

for (const [_key, item] of myMap) {
    item.method()
}

Judging the readability of equivalent syntax is subjective, but I'd argue the foreach is much more readable in this scenario as the method takes care of extracting the value from the Map rather than forcing you to deconstruct an array and ignore the key variable.

TLDR: The rule's claim that for...of is more readable is both highly subjective and situation dependent.

[mctrafik]

I disagree with the proposal here. I think forEach has 0 benefit and only downsides. Inlining everything, especially multiple statements into 1 line should not be a goal and isn't for most enterprise code.

I.e.

arr.map(someMap).forEach(doSomething)

Reads OK until it gets longer, and then you get an exception on that line, and you do not know where it happened because you crammed too much code into 1 line.

Doing a forEach at the end of a long chain, i.e. arr.filter().flatMap(..).filter(..).forEach(..) is only good for programming competitions and interviews, but is difficult to debug and it is harder to read.

Using a proper for-loop forces people to break their code up so that it's readable and debuggable.

Lastly, I'm tired of reviewing code from someone who loves forEach and write stuff like this:

function checkArray<T>(array: T[], condition: (i: T) => boolean): boolean {
  array.forEach(x => {
    if (condition(x)) return false;
  });
return true;
} 

and before you say this never happens, I've caught this error twice in last couple of years, and this is from devs who make $400k a year.

So I think forEach should continue to be banned by default.

[ematipico]

This rule has been removed from the recommended set, the change will be available in v2