Update: allow continue instead of if wrap in guard-for-in (fixes #7567)

[michaelficarra]

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

[ljharb]

Could this be behind an option? I want to preserve the current behavior (disallowing continue statements).

[michaelficarra]

@ljharb I don't think that'd be appropriate here. This rule is not stylistic. It intends to protect you from a potential hazard, and either pattern is safe.

[platinumazure]

I agree this should probably be behind an option. , and it seems if we want to allow continue, we should also allow break in the same way.

Let's discuss in the issue.

[ljharb]

Since the rule isn't checking what the guard is doing - ie, it's not enforcing a hasOwnProperty check - it's not actually protecting against a hazard. I've seen plenty of people accidentally bypass this rule by checking something besides hasOwnProperty.

[platinumazure]

I agree @ljharb, but I think that should be discussed as a separate issue. This is a rule that came from JSHint originally, and they had the same limitation there. (We could certainly improve the rule to actually be useful in this regard! I just wanted to explain the historical reason for its current state.)

…

On Jan 1, 2018 9:50 PM, "Jordan Harband" ***@***.***> wrote: Since the rule isn't checking *what* the guard is doing - ie, it's not enforcing a hasOwnProperty check - it's not actually protecting against a hazard. I've seen plenty of people accidentally bypass this rule by checking something besides hasOwnProperty. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9796 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWenqMRIIhuYsBhxNw9Dq_pSYQr1t4ks5tGadtgaJpZM4RQOcf> .

[michaelficarra]

@platinumazure break/return/throw wouldn't be used to avoid iterating prototype properties (while still iterating own properties) like continue is.

[platinumazure]

@ljharb Could you use no-restricted-syntax to catch the style that you don't want, while allowing this change to occur without a new option? Just wondering... We could certainly add a new option later if we needed to (i.e., if enough people asked for it).

[ljharb]

@platinumazure if we want to ban continue, then yes, but if we want to allow continue (just not in this style) then we couldn't. I think it should go behind an option.

[michaelficarra]

I still think that an option here is inappropriate for the reasons I mentioned above. This rule is listed in the Best Practices section, not the Stylistic Issues section. Using it to enforce a particular style is an abuse of this rule. And without an option, it still always protects you from the possible hazard.

[ilyavolodin]

I do agree with @michaelficarra here. This rule is in a different category then things like no-continue. It's not really meant for styleguide use (as far as I understand that's the problem @ljharb has with adding it in without an option). The style rule should be the one that allows/disallows continue inside specific nodes. I know we don't have available right now, but it can be created as a plugin.

[ljharb]

"styleguide use" covers all categories of rules; my issue with adding it without an option is that i want the entire body of the for loop to be surrounded by the same single condition, and i want the extra nesting, so that the existence of the condition is clear.

It is not a "best practice" to early-continue a for loop under a guard (nor is it a bad practice). In fact, it's simply @michaelficarra's preferred style here.

In terms of protecting against a hazard, this rule does not actually protect against the hazard - it simply requires any condition.

This rule is aimed at preventing unexpected behavior that could arise from using a for in loop without filtering the results in the loop.

The unexpected behavior is when you encounter an inherited property; filtering the results isn't a generic solution to that problem.

[platinumazure]

I don't see why we couldn't add an option here-- after all, @michaelficarra could certainly use the option in his own setup.

Can someone convince me why adding an option would be a bad idea?

[not-an-aardvark]

It makes the rule slightly more complicated for users to configure, and also slightly more complicated for us to maintain. Additionally, the option seems to be tangential to the main purpose of the rule. In this case I don't think it's worthwhile.

[michaelficarra]

@ljharb I'd love for the rule to be stricter about the condition of the guard, but I think that change is inappropriate for this PR. Also, "best practice" (probably not the best term) is defined on that page as

better ways of doing things to help you avoid problems

and I believe that both approaches allowed by this rule accomplish that goal.

edit: To emphasise my point, if there was a third way to reliably and generally protect against the enumerable prototype properties hazard, I would want this rule to allow that pattern as well, even if I did not personally plan to use that pattern.

[not-an-aardvark]

Looks good to me, thanks!

[platinumazure]

LGTM, thanks for contributing!