Set operator-linebreak rule to "before"

[Krinkle]

What

Currently required:

var x = isItGreat ?
  Example.might("be") :
  Example.or("not");

Proposed:

var x = isItGreat
  ? Example.might("be") :
  : Example.or("not");

History

JSLint famously enforced a (non-configurable) requirement that operators must obscured and dangle at the end of line, because this would avoid bugs in "old parsers" (presumably older minifiers and other tooling that for some reason did not or could not follow the ECMAScript spec correctly?). Evidence of which parsers suffered, appears to be lost to time. I tried to git-blame the jslint codebase, but its author doesn't write meaningful commit messages. Discourse elsewhere only repeats the vague claim above.

https://github.com/jshint/jshint/issues/60#issuecomment-806302

[Line breaks before operators] may cause problems like semi colon insertion and old javascript parsers breaking.

When JSHint came about, it inherited this stylistic quirk default, although it quickly introduced the laxbreak option to disable this. https://jshint.com/docs/options/#laxbreak

• laxbreak. Suppresses warnings about "possibly unsafe line breakings".

MediaWiki code has historically preferred leading symbols for readability. I believe (although I have no evidence of this) that this generally allows for faster comprehension and reduces ambiguity and cognitive overhead. It displays in a consistent visual position what operation is being performed, makes clear for ternaries which that the value is mutually exclusive and where this starts and ends. In particular, it avoids muddying the otherwise reasonable assumption that a line containing a literal expression, is its own used value. For example, multi-line arrays and objects (one value per line), multi-line function calls (one arg per line), and so on. In PHP, this remains our enforced coding style to date. https://www.mediawiki.org/wiki/Manual:Coding_conventions/PHP

When I adopted JSHint for MediaWiki in July 2012, this was our convention in JavaScript as well. I asked upstream JSHint how to make our coding style pass under JSHint. https://github.com/jshint/jshint/issues/557:

You should use laxbreak. Default behavior is (historically) what Crockford recommended.

And so we did! https://gerrit.wikimedia.org/r/c/mediawiki/core/+/14017

[mediawiki/core] jshint: add .jshintrc / .jshintignore files

"laxbreak": true

By 2014, we were using JSCS for most coding style rules, and likewise there we did not enforce line breaks after +, ?, :, &&, or || operators there either. In fact, the only thing we enforced was that for . chaining, the line break must be before it.

https://github.com/jscs-dev/node-jscs/blob/cf60723df58e435f39c7a769b7c74c1b8a3726a7/presets/wikimedia.json

Last version of our JSCS preset:

 "disallowOperatorBeforeLineBreak": ["."],

// Correct
var x = foo
  .bar()
  .baz();

// Incorrect
var x = foo.
  bar().
  baz();

In 2016, Oleg (former JSCS lead) created for us the first version of eslint-config-wikimedia, as part of merging JSCS into ESLint. I believe that he accidentally introduced this rule for us, perhaps based on misreading what we set disallowOperatorBeforeLineBreak to, or perhaps based on the historical JSLint/JSHint preference and presuming that as a common style preference. https://github.com/wikimedia/eslint-config-wikimedia/blob/v0.1.0/.eslintrc.json#L106-L108

"operator-linebreak": [
      "error",
      "after"
    ],

We deployed this a few weeks later and auto-fixed our code base. https://gerrit.wikimedia.org/r/c/mediawiki/core/+/321732

[mediawiki/core] build: Replace jscs+jshint with eslint

This ESLint rule originally did not reject ternary operators. Someone asked for it in 2015. https://github.com/eslint/eslint/issues/3274

Add support for detecting ternary operator in operator-linebreak rule.

ESLint lead Nicholas Zachas confirms: https://github.com/eslint/eslint/issues/3274#issuecomment-127799736

I think this might have been intentionally omitted because people tend to put the ? and : at the front of lines even when everything else is at the end.

The rule was changed in the next release to be able to enforce it both ways, with ternaries kept off by default.

https://github.com/eslint/eslint/issues/4294 https://eslint.org/docs/latest/rules/operator-linebreak

The default configuration is "after", { "overrides": { "?": "before", ":": "before" } }

Unfortunately, because projects tend to hardcoded the defaults, this meant anyone that configured ["error", "after"] would change their behaviour since setting this replaces any "default override". As an example the JavaScript Standard Style quickly applied this opt-out to avoid a regression after upgrading ESLint. https://github.com/standard/eslint-config-standard/commit/a78c4bbc56a792aad7c15d9c9016274254141012 Latest version: https://github.com/standard/eslint-config-standard/blob/v16.0.3/eslintrc.json#L194

fix regression with ternary operator handling

- "operator-linebreak": ["error", "after"],
+ "operator-linebreak": ["error", "after", { "overrides": { "?": "before", ":": "before" } }],

Ironically, Crockford himself has since abandoned this peculiar style in 2017. He now embraces the opposite, same as MediaWiki PHP and Standard JS. https://github.com/jslint-org/jslint/commit/c08484fb0559e205084e3c50194e40b7da21de8d

[jslint] break left

Proposal

https://eslint.org/docs/latest/rules/operator-linebreak

I suggest we do one of two things:

1. Set it to "before" as override for ? and :

This would match the default that ESLint and Standard JS already set.

/* eslint "operator-linebreak": ["error", "after", { "overrides": { "?": "before", ":": "before" } }] */

var x = isItGreat
  ? Example.might("be")
  : Example.or("not");

if (
    mw.foo.hasBar() &&
    mw.foo.getThis() === 'that' &&
    !mw.foo.getThatFrom( 'this' )
) {}

2. Set it to "before" in general

This would match what we do in MediaWiki PHP via PHPCS.

/* eslint "operator-linebreak": ["error", "before" ] */

var x = isItGreat
  ? Example.might("be")
  : Example.or("not");

if (
    mw.foo.hasBar()
    && mw.foo.getThis() === 'that'
    && !mw.foo.getThatFrom( 'this' )
) {}

[jdforrester]

This is fine, but it should be proposed on https://www.mediawiki.org/wiki/Manual_talk:Coding_conventions/JavaScript not here, for general developer community involvement (only three or four people actually watch this repo).

[Krinkle]

Done.

[thiemowmde]

I responded in https://www.mediawiki.org/wiki/Manual_talk:Coding_conventions/JavaScript. TL;DR: Please don't do number 2.