Manual talk:Coding conventions/JavaScript

Should we add a section about how to configure for ESLINT with the .js format? The .json format got removed in version 9, but I"m not comfortable making a decision to recommend version 9.

Related: phab:T364065

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?

something == null ? fallback : something is prohibited by the equality section and something ?? fallback is prohibited due to being too new. The next best way would be (typeof something === "undefined" || something === null) ? fallback : something, which is quite too much of a mouthful. Is there some shorter way I'm not aware of, or is this use case just too niche for the MediaWiki codebase?

If you know the expected type you could do e.g. !something && something !== ''.

Ooh, that is smart. It stunned my brain for a minute but it is short and nice. THanks.

In the next release we will be enabling the prefer-arrow-callback rule by default: https://github.com/wikimedia/eslint-config-wikimedia/issues/572

Chose option 2 and updated the page

Hi all, I'd like to discuss documentation comment conventions for classes as part of the migration to JSDoc. In JSDoc, specifically for classes:

• The main description in the documentation comment (or the @description tag) is interpreted as the description of the constructor
• The @classdesc tag is interpreted as the description of the class

These two description fields appear in the documentation site as:

# mw.MyClass
Class description

## Constructor
### new mw.MyClass()
Constructor description

There are two ways this can be formatted in the comment:

1. Constructor description first: Simpler but in reverse order from how the information appears on the documentation site

/**
 * Constructor description first line.
 * 
 * Constructor description continued...
 * 
 * @classdesc Class description first line.
 * 
 * Class description continued...
 * 
 * @param string myParam
 */

2. Class description first: More verbose but reflects the same order as the documentation site

/**
 * @classdesc Class description first line.
 * 
 * Class description continued...
 * 
 * @description Constructor description first line.
 * 
 * Constructor description continued...
 * 
 * @param string myParam
 */

The empty lines aren't required by JSDoc, but I've included them for readability and consistency with Manual:Coding conventions/Documentation.

Which option should we standardize on? Ideally, we'd be able to use @constructor instead of @description, but JSDoc doesn't support that. Of the two options, I think option 2 is the least ambiguous and easiest to reason about.

I think developers have a tendency to, whatever comes first in that block, is where we put high-level explainers, detailed descriptions, and usage examples for the class as a whole.

Having that be tucked away in the constructor description would be unfortunate and make the information less discoverable and thus waste investments in it somewhat.

For that reason, and for parity with PHP conventions wherever possible, I'd suggest option 2.

@APaskulin (WMF) I believe your code example is for the lower-level ("ES5-style") class, where the class and constructor are defined together as function FooBar(…) {…}.

When ES6 syntax sugar is used (which makes it look like the class and constructor are separate things), there is usually two separate doc blocks:

class FooBar {
 constructor() {…}
}

Can you add an example for this as well? Does it require the same tags? I'm hoping @description would no longer be needed in that case, and would @class stay in the first block?

That's a great point. This is much cleaner and works correctly:

/**
 * Class description.
 */
class myClass {
	/**
	 * Constructor description.
	 * 
	 * @param ...
	 */
	constructor() {...}
};

Adding a @class tag to the ES6 example (in either comment) actually messes it up and causes JSDoc to duplicate the class.

In the ES5 example, the @class tag isn't necessary in most cases, but it doesn't break anything. I included it because we have a lot of @class tags in MediaWiki core, but I'll update that post to remove it since it's not required.

 Done with wikimedia/eslint-config-wikimedia 0.26.0

With the advent of the frontend stable interface policy, there's a need to mark things as stable. This is especially true in the near-term as most frontend code doesn't have any indication of stability, so visibly seeing @stable tells the developer that method/class is there to stay.

JSDoc/JSDuck does not currently support @stable but a similar feature has been proposed. While our generated docs won't do anything special when they see this tag, it'd be nice to be able to use it without eslint yelling.

Thus, I'm proposing we allowlist @stable through the use of the definedTags option for the check-tag-names rule.

Later, once the new frontend stable interface policy has matured, we may wish to be more granular like we do for PHP and tag things as "@stable to call", "@stable to extend", etc., but for now just being able to use "@stable" at all (without having to add an ignore rule) I think is a good start.

Support Sounds like a good idea to me. Sam Wilson 05:40, 18 October 2023 (UTC)

Support Jdlrobson (talk) 18:39, 20 October 2023 (UTC)

The "one-var" rule was removed last year. See Topic:W5dh34nv202fzq7f for more.

I have been thinking and researching about the topic of multiple vs. single var statements recently and have changed my ways entirely. I switched from using var do declare bunches of variables at the same time to using one var per line. Here are some reasons, going so far that I would completely discourage from using a single var for anything more complex than var a, b, c;

• Multiple var are better readable in diffs. Each line ends with a ";" and is completely self-contained. E.g.:

var whatever = 0,     whatever2 = 2;	vs.	var whatever = 0;  var whatever2 = 2;
Two lines changed		One line changed

The multiple var version will therefore keep the git blame for the first line in tact, which is a huge advantage. Changing a line of code as a side effect of syntactical sugar or preferences is bad.

• Huge difference when debugging. If you have a list of five variables using one var and you want to see the value of the second one before creating the third one, then this is not really possible in any debugger known to me since all five variables are created in one step.
• Whenever tabs are displayed with 8 spaces, the whole one var thing looks incredibly ugly:

var foo = 123, bar = 321;

vs.

var foo = 123; var bar = 321;

• When assigning initial values does not fit into one line, one is left with the question how to spread the assignment over multiple lines and I have seen various models, none of which makes me feel comfortable when looking at. Some examples (there are many more variations, you get the idea I hope):

// Quick skimming over the code could give you // the expression "xxx" is a variable as well. var foo = foo( xxx + "someverylongstring..." ), anothervar = false;

// less misleading but still kind of ugly var foo = foo( xxx + "someverylongstring..." ), anothervar = false;

var foo = { field: 1 }, bar = { field: 2 };

vs.

var foo = { field: 1 }; var bar = { field: 2 };

or, adding tabs for formatting the first var as soon as the second gets added, sucks a lot when diffing again:

var foo = { ■■■■    field: 1 ■■■■};,     bar = {         field: 2     };

vs.

var foo = {     field: 1 }; var bar = {     field: 2 };

• Multiple var offer higher aesthetics for comments on top of variables without even thinking about it:

// some comment about foo var foo = true, // some come comment about bar bar = false; or (better, but haven't seen this one so often) // some comment about foo var foo = true, // some come comment about bar bar = false;

vs.

// some comment about foo var foo = true; // some come comment about bar var bar = false; Compared to the first multi var version this gives more space for comments and does not look odd or like the one comment is more important than the other one. Also, the first single var version would look quite horrible in a diff when removing the first variable declaration.

Most of this has also been covered in a more extensive article by Ben Alman.

I am not saying we should change all var statements now, but add this to the coding guidelines and have an eye for it during code reviews. Currently the convention does even discourage from using multiple var, I highly disagree with this.

Done.

Check eslint-config-wikimedia #360 on GitHub.

This landed last year; documentation updated.

Single quotes indeed.

What is the convention for quotes - double or single?

It has little meaning, but would be nice for consistency.

jQuery suggests double.

If I remember correctly it is single.

Single quotes indeed.

For JavaScript files quotation is purely stylistic as string literals in JavaScript are specified as either wrapped in double quotes or single quotes. There is no magic functionality or different escaping (e.g. no need for double quotes to use \n or something).

In PHP we use both because of magic quotes, though in PHP we also prefer single quotes when magic functionality is not intended.

To my surprise, I just got a hint from a fellow developer that I am not doing whitespaces as stated by the conventions. e.g. I am writing if(...) {} rather than if (...) {}. This has been legal before Revision 547172 as of 13:45, 6 June 2012. That revision has introduced the line Keywords that are followed by ( should be separated by one space. I think the given justification (This helps distinguish between keywords and function invocations) is not really a good one. First, you should really know your JS keywords, second, function calls are not followed by any "{" but by ";", so this should make it clear as well.

Since the not so spacey convention is (for a long time already) explicitly allowed by the PHP coding conventions (which were the general conventions not so long ago), I would like to demand bringing these on the same level again, allowing the not so spacey style in JS as well. As the PHP conventions put it, "Opinions differ" on this, and yes, that's no different when it comes to JavaScript. Please let me know if there are any objections, otherwise I would just adjust this soonish.

In general, I would appreciate if coding conventions could be discussed a little more, or perhaps someone could just point me to where they are being discussed... Thanks.

• Some operators require a space by syntax, others tolerate both (in the case of if, both are allowed). In that case it comes down to convention and consistency.
• Our JS code base is relatively new, therefore it doesn't make sense to start off with allowing arbitrary variations that are inconsistent in nature. Incorporating exceptions in the code conventions (the convention to separate all keywords by a space) might make sense for an established code base, but not for a new one.
• What do you mean by "know your keywords"? Are you suggesting someone might not know what is an operator and what a function? Though I think one should know what is and isn't an operator, that's easily looked up if you don't know, and then there is editors that do the highlighting for you, and of course the whitespace convention to make it easy to detect. And to be fair, it is a reasonable expectation to know the basic operators in a language before writing in it, of course (if, else, function, throw, ..).

As for discussion place, I think this talk page is as good as any.

Provided the following use cases for enforcing the "space after if":

• Consistency in style (sometimes a space and sometimes not is inconsistent)
• Consistency in syntax (some operators require it, some operators don't)
• Simplicity in distinguishing operators/keywords from function invocations.

Can you respond with a few use cases for leaving it "differed" ?

One could simply turn around your use cases if one would say that we are talking about spaces before parentheses rather than spaces after keywords, and perhaps that's actually the logical ting to do:

1. Consistency in style (sometimes a space before parentheses and sometimes not is inconsistent, e.g. someFunc() is very inconsistent with if (...) {...}).
2. Consistency in syntax (some operators require it, some operators don't) - Sorry, but I don't understand. I am not sure whether all keywords are operators, but not all operators are keywords. So we are not consistent here at all right now. E.g. ++i; rather than ++ i; or !someVar instead of ! someVar.
3. Simplicity in distinguishing operators/keywords from function invocations. So, that one is a contradiction to #1 now. But it's also not needed since you still have several indicators making clear whether the thing, preceding parentheses, is a keyword or a function-call:

• Your IDEs syntax highlighting (come on, probably all serious IDEs have this, even most modern debuggers do)
• Your knowledge (and yes, you understood me right when I said "know your keywords" :)
• Keywords followed by parantheses "( )" (control structures) will always be followed by braces "{ }" as well (per our own conventions where we don't allow Braceless control structures).
• Function calls should be followed by ";", control structures don't require this. The only exception here would be something like var foo = function() {};. This function example is actually also the only case where a keyword followed by parantheses, followed by braces, doesn't necessarily have subsequent, indented line.

I guess there could evolve a huge discussion around this and I am sure you are convinced that the current style is the one to go, I am convinced that this is the style which makes a lot more sense. For me the current style is just as inconsistent as the one I'd suggest is for you. There must have been a similar discussion when people decided over spacey or not so spacey style in PHP and perhaps we should learn from history and be consistent with PHP - it still is MW code and both languages have C-like syntax, so why not leave this rule to the common coding conventions. I believe the more rules are described on a common basis, the better. It makes things simpler and lets you concentrate on the essential things like actually writing code and getting stuff done rather than studying conventions for different syntactically very similar languages.

The only argument I'm willing to push is consistency for exact usage and the reason thereof:

• Not if() in one module and if () in another.
• We don't need to tolerate both variations, because unlike the PHP code base, the JS code base is relatively new and we can easily enforce one variation and stick to it.

The other arguments such as consistency among different operators are harder to document because, as you say, ++ and ! are also operators. So let's not argue that now. If you want you could specify it as follows:
"A keyword followed by ( (left parenthesis) should be separated by a space."

Which is effectively the same as what I meant with "Consistency in syntax (some operators require it, some operators don't)" because operators that don't require it are followed by a left parenthesis, and the ones that don't require a space by syntax.

Regarding "being consistent with PHP", we aren't inconsistent with PHP because our PHP code conventions don't enforce if(), it currently tolerates both. See my first point above.

Regarding "more common code conventions", I oppose this because of two reasons:

• It means we have to read two pages for a language's conventions instead of one.
• It encourages (and has led to) bad coding habits. Different languages have different syntaxes and internals, despite looking similar. Applying code structure conventions decoupled from the language is dangerous and harder to maintain and use. I've been working on phasing out those "common code conventions". For example, another PHP/JS project I work on preferred placing the curly place on the next line. This is fine in PHP where the distinction is purely stylistic, but in JS there are various cases where this changes the behaviour of the program (usually due to Automatic Semicolon Insertion).

I agree that "More rules" (that is, in order to removing ambiguity) is better. But more variations ("switch cases intended or not intended, if with or without space") or more edge cases ("foo like bar, except when bar is quuxified or when quuxification happens within baz") is not a good thing and serves no purpose what's however other than reducing consistency and asking for problems and conflict resolution. We stick with one and you get used to it.