Use `for…of` instead of `.forEach(…)` typescript:S7728, javascript:S7728

The addition of this rule is an absolute head-scratcher. The supporting statements I’ve seen on the rule’s page and in threads about it don’t make sense.

From another thread:

But for most cases, the advantages of a for…of outweigh the minor (in majority of situations) performance differences, especially when you need control flow (break/continue), async/await support, or want to iterate over non-array iterables like Maps and Sets.

If you need control flow, you can chain a filter on your array before iteration to trim it down to just usable items or you don’t use forEach, it’s up to the developer to use the right tool for the job.

Why not simply flag async/await inside forEach or disallow non-void async calls? Nearly every chainable array method will have the similar unexpected behavior with async/await. ESLint has had no-await-in-loop as an optional rule since early in version 3, and @typescript-eslint/no-misused-promises covers the specific await in forEach scenario as well as many other promise-related bugs. You could even suggest for…of for this scenario, as it’s async safe. Not worth throwing the whole tool away because someone might use it wrong.

Iterating over non-array iterables isn’t relevant to this rule. From the rule page: “This rule raises an issue when the forEach method is called on arrays…” It’s worth noting that both Set and Map have forEach methods that work quite well in the same way, which makes it even stranger that they would be brought to argue against forEach. While there are scenarios where iterating with for…of is preferable to something like casting to an array first, those aren’t exactly a common situation and could be caught by checking if something’s being cast to an array before iteration.

I’ve never had an issue with TS losing type information inside an array.forEach. Ever. I’m not saying it can’t happen, but it’s exceedingly rare and I would expect it to be a symptom of code issues elsewhere.

As for readability for “developers coming from other programming languages”, I’m curious what languages these imagined developers used. Array.forEach with a similar syntax is part of Java, Rust, Ruby (.each), PHP, and many more. I can’t think of an experienced developer who would be new to JS/TS in 2025 who hasn’t written one of those other languages, at least not one who wouldn’t struggle with most of the syntax differences between JS/TS and their language of choice.

This…
for (const [index, element] of list.entries()) {
console.log(`${element} of ${index}`);
}

…is not as easy to read as this:
list.forEach((element, index) => console.log(`${element} of ${index});

The rules page refers to the latter as “less intuitive syntax for simple iterations”, which must be using some definition of the word “intuitive” that I wasn’t previously aware of. That point made, it again is a stylistic choice, not a Code Quality Issue. The stylistic consideration should be for the developers actively working in the language, not some potential future developer who’s smart enough to understand the first example but also incapable of grasping the second.

It simply doesn’t make sense why this rule would be added at all, let alone rolled into the core SonarWay ruleset. It was known to be controversial and opinionated, it goes against well over a decade of standard practice, and it completely throws away an extremely useful tool in exchange for a clunky, slower, and overly verbose alternative that’s only better for a handful of scenarios.

When you drop the edge cases and scenarios that should be handled by a more granular rule, there’s no advantages “in most cases” to justify disallowing .forEach in favor of for…of. It’s not significantly more performant (until your array is in the millions of items), it’s not more readable, it’s not more maintainable, and there’s really no good “code quality” reason for it.

If you want to remove an Array method that actually leads to unreadable, unmaintainable, cognitive overload code, kill Array.reduce. It’s available in the same eslint plugin you took this rule from: eslint-plugin-unicorn/no-array-reduce

2 Likes

Hello @sickhippie,

thank you for this feedback, this is very valuable for us. When we did the effort to select the best suited rules for Sonar from the unicorn plugin, this was indeed the single rule where we were most doubtful about. I will still disagree with you about this:

But this again just proves that the rule itself is very opinionated and subjectivity plays a big part on it.

I very much agree on this point you made at the beginning of the post:

Let me have a discussion with the team and come back to you. Given the recent feedback we’ve had about this rule, I’m more inclined to remove it from Sonar way and let the user decide if enabling it or not.

2 Likes

As a follow up, this rule is being removed from Sonar Way in the next release. It will be deployed in SonarQube Cloud in the coming days.

3 Likes