Noissue - Add some more JavaScript coding guidlines

pages/coding-style.html

 title=Coding Style
 
 
 <h2 id="precedence">{{s1 Precedence}}</h2>
 
 <p>{{s2 The Adblock Plus coding style is based on others. Our rules (both general and language specific) always overrule referenced style guides. The language specific sections overrule the general section.}}</p>
 
 <h2 id="consistency">{{s3 Consistency}}</h2>
 
 <p>{{s4 Consistency is most important: Be consistent within functions, files, modules and projects. Making existing code conform with this style guide is fine, but it should happen in dedicated commits, preferably for a whole module or project at once.}}</p>
 
 <h2 id="general">{{s5 General}}</h2>
 <ul>
   <li>{{s6 Follow the Mozilla Coding Style's <a href="https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#General_C.2FC.2B.2B_Practices">general practices</a> and its <a href="https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Naming_and_Formatting_code">naming and formatting rules</a>.}}</li>
   <li>{{s8 All files should have a <a href="license-headers">license header</a>, but no mode line comments.}}</li>
   <li>{{s9 Newline at end of file, otherwise no trailing whitespace.}}</li>
   <li>{{s10 Lines can be longer than the limit, if limiting line length would hurt readability in a particular case.}}</li>
   <li>{{s11 Opening braces always go on their own line.}}</li>
   <li>{{s12 No hungarian notation, no special variable name prefixes or suffixes denoting type or scope. All variable names start with a lower case letter.}}</li>
   <li>{{s13 Don't comment code out, delete it.}}</li>
   <li>{{todo-comments Don't add TODO comments, create an <a href="https://issues.adblockplus.org">issue</a> and reference it in a comment instead.}}</li>
 </ul>
 
 <h2 id="javascript">{{s14 JavaScript}}</h2>
 <ul>
   <li>{{s15 Follow the Mozilla Coding Style's <a href="https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#JavaScript_practices">JavaScript practices</a>.}}</li>
-  <li>{{s16 Opening braces of object literals in don't go on their own line when that would cause a syntax error.}}</li>
-  <li>{{s17 Use <code>bind()</code> to ensure the desired value of the <code>this</code> variable, don’t use temporary variables as a replacement.}}</li>
+  <li>{{javascript-opening-braces Opening braces of object literals don't go on their own line when that would cause a syntax error.}}</li>

[kzar, 2017/01/17 06:48:52]

(I noticed an extra word "in" in this rule.)

   <li>{{s18 In classes, prefix private functions with a single underscore to make them pseudo-private.}}</li>
+  <li>{{javascript-block-scoping If supported, use <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Statements/block">block-scoping</a> (<code><fix>let</fix></code>), except when sharing global variables between scripts cannot be avoided.}}</li>

[Sebastian Noack, 2017/01/17 22:31:38]

The MDN page linked here explains code blocks in general. Perhaps we should
directly link to the section that explains block scoping with let and const:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/...

[kzar, 2017/01/18 06:33:51]

Done.

+  <li>{{javascript-arrow Use the <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Functions/Arrow_functions">arrow function</a> syntax when passing anonymous functions when the binding of <code><fix>this</fix></code> and <code><fix>arguments</fix></code> is not required.}}</li>

[Sebastian Noack, 2017/01/17 22:31:38]

Two times "when" reads quite weird too me. Also I still find the last part a bit
confusing. How about this:

  If supported, use the arrow function syntax when passing anonymous functions
  that don't need to access their own `this` or `arguments`.

[kzar, 2017/01/18 06:33:51]

Done.

+  <li>{{javascript-this When required (and when arrow functions cannot be used) use <code><fix>bind()</fix></code> to ensure the desired value of the <code><fix>this</fix></code> variable, don’t use temporary variables as a replacement.}}</li>

[saroyanm, 2017/01/17 12:26:05]

bind is not supported in IE8, websites does support IE8 and apparently we still
support IE8 https://adblockplus.org/requirements#internet-explorer

[Sebastian Noack, 2017/01/17 22:31:38]

Note that this rule isn't new (see s17 above). It just has been slightly
modified and moved here in order to fit in with the new rules. But perhaps we
should consider removing this rule altogether. For code that only needs to be
compatible with modern browsers (i.e. adblockplus, adblockpluschrome,
adblockpluscore, adblockplusui) it isn't relevant anymore as we have and prefer
arrow functions there now, which implicitly bind `this` to the outer context.
And for our websites, from your comment it sounds that it isn't relevant either,
as we have to deal with legacy browsers that don't even have bind() there.

[kzar, 2017/01/18 06:33:52]

I agree let's just remove this rule, it's not really relevant any more.

+  <li>{{javascript-method Use the <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Functions/Method_definitions">method definition shorthand</a> syntax when defining methods on an object.}}</li>

[saroyanm, 2017/01/17 12:26:05]

I think this applies only to where it's supported.

[Sebastian Noack, 2017/01/17 22:31:38]

Yeah, I think we should prefix all rules that require ES6+ features
with "If supported, " like you already did for some of them.

[kzar, 2017/01/18 06:33:52]

Good point Manvel, I moved all the modern JavaScript rules to their own section
to avoid confusion.

+  <li>{{javascript-strict Always use <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Strict_mode">strict mode</a>.}}</li>

[Sebastian Noack, 2017/01/17 22:31:38]

This being a fundamental rule (not addressing a specific case) perhaps we should
move it to the top just below referencing Mozilla's coding practices.

While on it, perhaps move all rules, that depend on ES6+ features, and should
start with "If supported, " as per the previous comment, to the bottom.

[kzar, 2017/01/18 06:33:51]

Yea good idea, I've done that but gone one better and created a new section for
the modern rules.

+  <li>{{javascript-if-else-braces When an <code><fix>if</fix></code> statement, an <code><fix>else</fix></code> statement or a loop spans over more than one line always enclose it with braces. When an <code><fix>if</fix></code> or <code><fix>else</fix></code> statement uses braces the opposing block should do too.}}</li>
+  <li>{{javascript-for-of If supported, use <code><fix>for..of</fix></code> when iterating over arrays in canonical order, rather than using <code><fix>for(;;)</fix></code> or the <code><fix>forEach()</fix></code> method. (Excepting when the loop index is required.)}}</li>

[Sebastian Noack, 2017/01/17 22:31:38]

How about addressing the loop index case together with the other requirement,
rather than adding an exception in the end?

  If supported, use `for..of` when iterating over arrays in canonical order
  and there is no need to access the loop index, rather than ...

Also, this still being a new JavaScript feature, we should link "for..of" to the
corresponding MDN documentation.

[kzar, 2017/01/18 06:33:51]

Done.

+  <li>{{javascript-object-proto Use the <code><fix>Map</fix></code> or <code><fix>Set</fix></code> data types rather than <code><fix>Object.create(null)</fix></code> when possible. (Never use <code><fix>{__proto__: null}</fix></code> and otherwise avoid the use of <code><fix>__proto__</fix></code>.)}}</li>

[Sebastian Noack, 2017/01/17 22:31:38]

As per MDN documentation the terminology is Map/Set object, not data types. Also
people who read this might not even be aware of Object.create(null) but would
just use {}. Moreover, {__proto__: null} might still be necessary for code that
has to be compatible with IE 8. How about this:

  If supported, use `Map` or `Set` objects, rather than misusing
  plain objects, when a hash table is needed. However, if you have
  to resort to plain objects for compatibility with legacy browsers,
  make sure to create the object without prototype, preferable using
  `Object.create(null)`, in order to avoid potential collisions.

Also please link "Map" and "Set" to their MDN documentation.

[kzar, 2017/01/18 06:33:52]

I've split up the rule talking about `__proto__` from this one, since half is
for modern JavaScript and you're right that mixing the two is kind of confusing.

I realise that the Mozilla docs say "object" rather than "data type" but I think
either is correct and in this context "data type" is easier to understand. (Kind
of like your other point about using the word "when" twice in a sentence I
guess.) But if you feel this is incorrect and insist I can still change it.

 </ul>
 
 <h2 id="html-css">{{html-css HTML and CSS}}</h2>
 <ul>
   <li>{{html-css-general Follow the <a href="https://google.github.io/styleguide/htmlcssguide.xml">Google HTML/CSS Style Guide</a>.}}</li>
   <li>{{html-css-quotation Use double over single quotation marks.}}</li>
   <li>{{html-css-optional-tag Don't omit optional HTML tags.}}</li>
   <li>{{html-css-hexadecimal CSS color values should be specified in hexadecimal where possible.}}</li>
   <li>{{html-css-shorthand CSS shorthand properties usage is optional.}}</li>
   <li>{{html-css-order CSS rule declaration order should follow the <a href="https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/#property-ordering">WordPress CSS Coding Standards</a>.}}</li>
@@ skipping 35 matching lines @@
 <ul>
   <li>{{objc-conventions Follow Apple's <a href="https://developer.apple.com/library/ios/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/Conventions/Conventions.html">conventions</a>}}.</li>
 </ul>
 
 <h2 id="puppet">{{s31 Puppet}}</h2>
 <ul>
   <li>{{s32 Follow the <a href="http://docs.puppetlabs.com/guides/style_guide.html">Puppet Style Guide</a>.}}</li>
   <li>{{s33 Opening braces don't go on their own line.}}</li>
   <li>{{s34 Arrows should not be aligned.}}</li>
 </ul>