Use guard-for-in eslint rule

[erikvold]

I didn't see an issue for this and I think it would be handy to use the guard-for-in in our eslint config.

​https://eslint.org/docs/rules/guard-for-in

[sebastian]

So this is about for..in including inherited properties (but only if they are enumerable). This is exactly the behavior I'd expect. I don't see how that is generally an issue.

[mjethani]

I should add that if one wants to only iterate over the object's own enumerable properties one can use Object.keys(), however this performs slightly worse than for..in. If we're going to then add a check for {}.hasOwnProperty(), it would be even worse. Either choose for..in or Object.keys(), this rule doesn't make sense (like so many ESLint rules), especially for adblockpluscore (but I see that the issue is for the Automation module).

[sebastian]

Replying to mjethani:

(but I see that the issue is for the Automation module).

Well, the codingtools repository which contains eslint-config-eyeo is owned by the Automation module, but the configuration is used by all repositories (including adblockpluscore).

Anyway, I agree with you. I will wait for Dave's feedback, and if he agrees too, I will reject this issue.

[mjethani]

OK, in that case I am one hundred percent sure we should not do this. I would never write a for..in loop that then calls {}.hasOwnProperty() within the loop since there is already Object.keys() for that, which is designed specifically for that.

Note that we do use for..in instead of Object.keys() if it performs better and where we know that there are no enumerable properties further up in the prototype chain, but then this rule would destroy the performance benefit of doing so (Object.keys() would perform better), which again means that it makes no sense to have this rule.

[erikvold]

Replying to mjethani:

OK, in that case I am one hundred percent sure we should not do this. I would never write a for..in loop that then calls {}.hasOwnProperty() within the loop since there is already Object.keys() for that, which is designed specifically for that.

Note that we do use for..in instead of Object.keys() if it performs better and where we know that there are no enumerable properties further up in the prototype chain, but then this rule would destroy the performance benefit of doing so (Object.keys() would perform better), which again means that it makes no sense to have this rule.

I see the rule as a means to prevent us from using for in, since if we hit the rule we can think about better implementations, though maybe a rule to prevent it's use would be better. I'd have to look at a use case you mention above to see if there is a better solution, fwict that use case may be overly relying on a particular inheritance structure.

[mjethani]

There are legitimate reasons to use for..in (when you want all the enumerable properties), wouldn't this prevent that? The rule seems to assume, from what I can tell, that there is never a legitimate use case for for..in where you want all the enumerable properties, including those further up in the prototype chain.

[erikvold]

Replying to mjethani:

There are legitimate reasons to use for..in (when you want all the enumerable properties), wouldn't this prevent that? The rule seems to assume, from what I can tell, that there is never a legitimate use case for for..in where you want all the enumerable properties, including those further up in the prototype chain.

Yes it will "prevent" that pattern you mention, exceptions can be added and the rule only really checks that there is an if statement after the for in statement. I think the rule is meant for those that want to avoid problems arising from inheritance which can be troublesome when dealing with outside code.

[mjethani]

I see, well in that case the rule should enforce the use of Object.keys(), like I said it is semantically equivalent to having a {}.hasOwnProperty() check but is faster and designed specifically for this use case.

[kzar]

I'm not a module owner for Automation, but FWIW my vote is to not add this ESLint rule. Any opinion Thomas?

[sebastian]

I'm generally against rules that flag legitimate practices, and require additional boilerplate (i.e. inline ignores) in order to keep using them, essentially just making the syntax artificially verbose.

Furthermore, I agree with Manish that even in cases where iterating over inherited properties isn't intended, that rule is suggesting bad practices (you should just use Object.keys() there instead).

Also I don't see a real thread of bugs through unintentional introspection of inherited properties. The whole point of inheritance is that properties are inherited, so it should come with no surprise that the canonical way to iterate over an object's properties will include inherited properties.

Replying to kzar:

I'm not a module owner for Automation

Again, while eslint-config-eyeo is maintained by the Automation module, it should reflect the coding style enforced by those that review any code linted by it.