Noissue - Add some more JavaScript coding guidlines

pages/coding-style.html

Index: pages/coding-style.html
diff --git a/pages/coding-style.html b/pages/coding-style.html
index 553402cd508a746e0587e41d989f54f1b2d588b0..3b734d771f8a84b1854ec58f792eb94cfce4c9b8 100644
--- a/pages/coding-style.html
+++ b/pages/coding-style.html
@@ -24,9 +24,16 @@ title=Coding Style
 <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>