Parenthesize mixed expressions of || and &&

[jgonggrijp]

ExtendScript apparently gives equal precedence to the || and && operators, so when the || appears before the &&, we need to anchor the order of evaluation with an extra pair of parentheses. See #2949.

@RaymondClr Could you test whether all of your code works correctly in ExtendScript with https://github.com/jgonggrijp/underscore/raw/extendscript-precedence/underscore-umd.js?

[coveralls]

Coverage remained the same at 95.217% when pulling c4e0920 on jgonggrijp:extendscript-precedence into 0557e33 on jashkenas:master.

[captbaritone]

Some thoughts (feel free to ignore if they don't seem helpful):

If we want to support this long term, there's probably an ESLint rule we could/should enable to ensure we don't regress.

Another option here would be to add these as part of our build step. I know we try to keep our build step as minimal as possible, so I can also understand the value of doing it in the code itself. This thought occurred to me because, presumably our minified bundle will strip these new parentheses.

[jgonggrijp]

Excellent comments, thanks @captbaritone! In one short post, you mentioned three things I didn't think of:

• Try whether parentheses can be added in a build step.
• Failing that, try to enforce them through an eslint rule.
• Parentheses must not be stripped during minification (though if we can't prevent this, I guess ExtendScript users can fall back to the unminified code).

I plan to go through the above list within the current PR.

[jgonggrijp]

@captbaritone Outcome of our discussion:

• The obvious way to add the parentheses in a build step would be by applying the fix option of the eslint plugin for the Rollup bundler. That basically means that eslint comes into play in any case and that the question remains whether the rule can be automatically fixed.
• The rule in question is no-mixed-operators, which unfortunately eslint cannot fix automatically. Nevertheless, I added the rule and configured it such that only mixes of && and || are disallowed. CI will fail if people submit PRs with mixed expressions.
• It turns out that terser was already adding parentheses to mixed expressions, so nothing needs to change with respect to minification.

[jgonggrijp]

@RaymondClr I'm just going to merge this now on the assumption that this solves #2949. However, please still double-check that the issue is solved on your end when you find the time. Thanks in advance!

[captbaritone]

Awesome. Thanks for the write up! Sounds like a great solution.

[captbaritone]

It might be worth adding a comment to the ESLint config explaining why the rule is enabled. Otherwise we might opt to remove in the future assuming it's purely stylistic without realizing it's load bearing.

[jgonggrijp]

Oops, good point. I assumed this wouldn't be allowed because the config is in JSON, but according to the eslint docs, it is allowed. I'll add it it in a separate commit on master.