Topic on Manual talk:Coding conventions/JavaScript

Currently required:

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

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

Proposed:

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

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

In summary:

• Consistency with Standard JS, ESLint default, and JSLint; which nowadays all enforce the "before" style.
• Consistency with MediaWiki PHP.
• Improved readability.
• The ESLint rule to enforce this "after" style came from JSHint, which inherited it from JSLint. It existed in JSLint to support "bugs in old parsers". During the time this concern was theoretically relevant (2009-2016), we actually had it disabled and used the before style instead as we favour readability (and consistency with PHP). In 2016, the auto-fixed transition from JSCS/JSHint to ESLint, accidentally flipped this rule and started enforcing the legacy "after" JSLint style.
• Even JSLint never used this style for ternaries, only for conditionals and concatenation. The reason we enforce it on ternaries is that ESLint changed the rule at some point (with a default opt-out), and we forgot to sync our config. Another auto-fix.
• In 2017, JSLint removed their rule completely, and now enforces the same "before" style that I propose here.

History:

Between 2009-2016, the proposed style was our style in MediaWiki JS code with the operator at the start of the line (and thus line break "before" the operator), same as in PHP.

This seemingly changed by accident in 2016 the JSCS Team helped us migrate from JSHint+JSCS to ESLint. From what I can tell, they misread one of our JSCS preset keys and added this style. This appears to be based on a legacy recommendation by Crockford (author of JSLint) to avoid bugs in "old parsers" that did not implement the ECMAScript spec correctly and for some reason had trouble with line breaks before certain operators. jshint issue #60 (2011):

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

When we adopted JSHint for MediaWiki in 2012, we had the same convention in both PHP and JavaScript. I asked upstream at the time, how to make our coding style pass under JSHint. jshint issue #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"
  "laxbreak": true

The last version of wikimedia JSCS preset in 2014 contains "disallowOperatorBeforeLineBreak": ["."], which requires line breaks before, not after, and only for the . operator.https://github.com/jscs-dev/node-jscs/blob/cf60723df5/presets/wikimedia.json

"operator-linebreak": ["error", "after"],
// Correct
var x = foo
  .bar()
  .baz();

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

The JSCS project merges into ESLint in 2016. The wikimedia preset used to be maintained within the JSCS project, and Oleg made a separate one for us at eslint-config-wikimedia v0.1.0 which set the operator-linebreak rule to enforce line breaks after operators. We deployed this a few weeks later and auto-fixed our code base:

• "[mediawiki/core] build: Replace jscs+jshint with eslint." https://gerrit.wikimedia.org/r/321732 (2016)

Now, at first, the ESLint operator-linebreak rule was mostly for operators in multi-line values (e.g. + concatenation) and multi-line conditionals (e.g. &&).

In 2015, someone requested a feature in upstream ESLint to be able to enforce the legacy JSLint style on ternary operators. This despite not even JSLint itself doing this for ternary operators (I guess the "old parsers" didn't have an issue with ternary operators?). ESLint lead Nicholas Zachas confirms:

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 next ESLint release added support for enforcing this odd style on ternaries, but, because it is believed to be unusual and to avoid regressions, upstream included a "default override". eslint issue #4294

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

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

Unfortunately, because projects tend to hardcode the defaults, this meant anyone that configured ["error", "after"] would see their behaviour change during an upgrade, because setting shadows any "default override". For example the JavaScript Standard Style quickly applied this opt-out to avoid a regression after upgrading ESLint. eslint-config-standard@a78c4bb, eslint-config-standard@v16.0.3

fix regression with ternary operator handling

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

Ironically, Crockford himself has long abandoned this peculiar "after" style in 2017. JSLint (still around!) now embraces the "before" style, same as MediaWiki PHP and Standard JS. jslint-org/jslint@c08484f

[jslint] break left

Proposal:

/* 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' )
) {}

Pull request: https://github.com/wikimedia/eslint-config-wikimedia/issues/591

Highly agree with "faster comprehension" 👍

Conditionals

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

I don't think this is enforced in phpcs, because 1) I see examples of the opposite of this in a coding conventions example (Manual:Coding conventions#Line width, code block four) and 2) I see examples of both styles in a random file I spot checked (EditPage.php, example 1, example 2)

The closest thing I could find in the coding conventions doc was Manual:Coding conventions#Line width, The operator separating the two lines should be placed consistently (always at the end or always at the start of the line), so it appears to not have an opinion on it.

Ternaries

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

Same as above. I am finding examples of both in php (EditPage.php, example 1, example 2), suggesting that phpcs does not have a rule about this. Additionally, I tested eslint 8 with the settings

{
  "extends": "eslint:recommended",
	"parserOptions": {
		"ecmaVersion": "latest",
		"sourceType": "module"
	}
}

and I was not able to get it to error for either type of ternary, suggesting to me that this may not be an eslint recommended rule.

Conclusion

Perhaps we should remove the eslint rule operator-linebreak from eslint-config-wikimedia completely, so that phpcs, eslint, and coding conventions are in alignment, that alignment being not requiring either style.

@Novem Linguae Do you prefer to place operators in JavaScript in different places at different times?

I'd like to understand what people think they need, and why. I understand you think we don't have a convention for PHP code, but I don't think we should decide based on that. We should decide based on whether there's value in it. Generally speaking, code comprehension is helped by reducing ways to express the same thing.

There are of course aspects of code authoring where it is useful to express things multiple ways to convey different subtle meanings. Such as where and whether to add a blank line between statements, or whether to use a if-else conditional or a ternary expression. Search for declined Codesniffer tasks to find examples such as T179768, T200629, T253914.

Manual:Coding conventions is a shared page about both PHP and JS. The first code block at Manual:Coding conventions#Line width contains PHP code, and uses the "before" style that is conventional in MediaWiki PHP. The fourth code block there contains JavaScript code, and uses the "after" style currently enforced by ESLint.

On the page about PHP coding conventions, it says Manual:Coding conventions/PHP#Ternary operator:

[…] the question mark and colon should go at the beginning of the second and third lines and not the end of the first and second

There is also the matter of having experience with the unfortunately enforced style in JavaScript affecting our muscle memory, with no enforcement on the PHP side. This makes checking existing code biased. Hence I'm asking for input on people's subjective experiences separate from what "exists".

To my knowledge, there is no interest or need for placing ternary symbols in different places at different times. The lack of enforced consistency in PHP is not because we don't want to enforce it, but we because we can't. PHPCS lacks built-in support for enforcing either way. T253187, PHP_CodeSniffer issue 3377, and T116561.

I'm in favour of switching to "operator-linebreak": ["error", "before" ], per Krinkle's comments above.

Unfortunately the discussion is a little mood because the ternary operator should rarely be used when it spans more than a single line. I mean, I like the operator and regularly use it. But only when the two branches are trivial (e.g. a single method call) and easy to grasp.

While I, personally, like to place ? and : at the beginning of the line I ultimately don't mind much – as long as I'm allowed to use the same style in PHP and JS.

What I care much more about is the placement of || and && in an if. I always put these at the end of the line for several reasons. I wouldn't be happy when some sniff would start to enforce the opposite.

"I always put these at the end of the line for several reasons"

Can you elaborate on these?