Issue 29371951: Noissue - Add some more JavaScript coding guidlines (Closed)
DescriptionNoissue - Add some more JavaScript coding guidlines
Patch Set 1 #
Total comments: 23
Patch Set 2 : Addressed feedback #
Total comments: 17
Patch Set 3 : Addressed feedback, reordered rules added const rule #
Total comments: 10
Patch Set 4 : Addressed feedback #
Total comments: 4
Patch Set 5 : Addressed further feedback #
Total comments: 3
MessagesTotal messages: 14
Patch Set 1 As discussed in the other review[1]. While at it I've added some other guidelines that came to mind. [1] - https://codereview.adblockplus.org/29362526/#msg7
https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:28: <li>{{javascript-this Use <code>bind()</code> (or arrow functions) to ensure the desired value of the <code>this</code> variable, don’t use temporary variables as a replacement.}}</li> For code snippets please also use the <fix> tag, see the Python section, for reference. This makes sure they get not translated. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:28: <li>{{javascript-this Use <code>bind()</code> (or arrow functions) to ensure the desired value of the <code>this</code> variable, don’t use temporary variables as a replacement.}}</li> I think this should go below the rule for arrow functions and clarify that you should only use the function statement + bind() for compatibility with legacy browsers if using arrow functions isn't an option. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:30: <li>{{javascript-let Use <code>let</code> or better yet <code>const</code> for variable definitions where possible instead of <code>var</code>.}}</li> I find this rule a little too vague. How about: If supported, use <a href="https://developer.mozilla.org/../Statements/let">block-scoping</a>, except when sharing global variables between scripts cannot be avoided. As for const, this should could go into another rule, if we come up with any. But keep in mind that we currently only use const for immutable types. If we would use const wherever possible we would also have to use them for objects which might change, as long as the variable itself isn't reassigned. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:31: <li>{{javascript-arrow Use the <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Functions/Arrow_functions">arrow function</a> syntax for anonymous functions when the binding of <code>this</code> and <code>arguments</code> is not required.}}</li> Arrow functions automatically bind "this". So it seems your implications here are the wrong way around. The reason to not use arrow functions would be because NOT binding it would be required, right? https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:32: <li>{{javascript-method Use the <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Functions/Method_definitions">method definition</a> syntax where possible, even in preference to arrow functions.}}</li> "method definition" is ambiguous as the old syntax (i.e. |{method: function() { ... }}|) arguably is a form of method definition as well. However, we a re specifically talking about the method definition _shorthand_ syntax here. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:32: <li>{{javascript-method Use the <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Functions/Method_definitions">method definition</a> syntax where possible, even in preference to arrow functions.}}</li> I don't like how we overrule another rule here (i.e. "even in preference to arrow functions"). Perhaps a better way to put it; "use the arrow function syntax when passing anonymous functions" and "use the method definition shorthand syntax when defining methods on an object". That way we have a clearer distinction and mutually exclusive definition of the use cases, without need for any exceptions. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:33: <li>{{javascript-strict Where possible always use <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode">strict mode</a>.}}</li> Why the "where possible" restriction? Is there any legit use case for some behavior that only exists outside of strict mode? https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:34: <li>{{javascript-if-else-braces When an <code>if</code>, <code>else</code> or <code>loop</code> statement spans over more than one line always enclose it with braces. When an <code>if</code> or <code>else</code> statement uses braces the other half should do too.}}</li> "other half" doesn't sound too technically accurrate, how about "opposing block"? https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:34: <li>{{javascript-if-else-braces When an <code>if</code>, <code>else</code> or <code>loop</code> statement spans over more than one line always enclose it with braces. When an <code>if</code> or <code>else</code> statement uses braces the other half should do too.}}</li> "loop" isn't a literal code snippet, so it shouldn't use the <code> tag. Also "loop" is not a statement (but a group of statements). When an <code><fix>if</fix></code>, <code><fix>else</fix></code> or statement or loop spans over multiple lines ... https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:35: <li>{{javascript-for-of When iterating through Arrays and similar prefer <code>for (let item of array)</code> loops to traditional <code>for (let i=0; i < array.length; i++)</code> or even <code>array.forEach(...)</code> unless there is a good reason to do otherwise.}}</li> I think we are really only talking about arrays here, as there don't seem to be any other objects which support both, length+indices as well as the iterator protocol. Also I wonder whether we can come up with a less vague rule. The only "good reasons" to not use for..of would be if you need to either access the the loop index, or have to iterate in non-standard order (skipping the first/last, reversing order, etc.), or if it isn't supported by any targeted browser. How about this: If supported, use for..of when iterating over arrays in canonical order, rather than using for(;;) or the forEach() method. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:36: <li>{{javascript-object-proto Use <code>Object.create(null)</code> instead of <code>{__proto__: null}</code> and otherwise avoid the use of <code>__proto__</code>.}}</li> Well, by now, you should rather use Map or Set objects if you need a hash table, rather than using Object.create(null) to avoid potential collisions.
Patch Set 2 : Addressed feedback https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:28: <li>{{javascript-this Use <code>bind()</code> (or arrow functions) to ensure the desired value of the <code>this</code> variable, don’t use temporary variables as a replacement.}}</li> On 2017/01/16 17:07:14, Sebastian Noack wrote: > For code snippets please also use the <fix> tag, see the Python section, for > reference. This makes sure they get not translated. Done. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:28: <li>{{javascript-this Use <code>bind()</code> (or arrow functions) to ensure the desired value of the <code>this</code> variable, don’t use temporary variables as a replacement.}}</li> On 2017/01/16 17:07:14, Sebastian Noack wrote: > I think this should go below the rule for arrow functions and clarify that you > should only use the function statement + bind() for compatibility with legacy > browsers if using arrow functions isn't an option. Done. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:30: <li>{{javascript-let Use <code>let</code> or better yet <code>const</code> for variable definitions where possible instead of <code>var</code>.}}</li> On 2017/01/16 17:07:14, Sebastian Noack wrote: > I find this rule a little too vague. How about: > > If supported, use > <a href="https://developer.mozilla.org/../Statements/let">block-scoping</a>, > except when sharing global variables between scripts cannot be avoided. > > As for const, this should could go into another rule, if we come up with any. > But keep in mind that we currently only use const for immutable types. If we > would use const wherever possible we would also have to use them for objects > which might change, as long as the variable itself isn't reassigned. Done. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:31: <li>{{javascript-arrow Use the <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Functions/Arrow_functions">arrow function</a> syntax for anonymous functions when the binding of <code>this</code> and <code>arguments</code> is not required.}}</li> On 2017/01/16 17:07:14, Sebastian Noack wrote: > Arrow functions automatically bind "this". So it seems your implications here > are the wrong way around. The reason to not use arrow functions would be because > NOT binding it would be required, right? Well I think arrow functions simply close over the binding of `this` and `arguments` from the parent scope, the same as functions normally do for other variables. I think that's because they simply don't bind `this` and `arguments` to something new like old style functions do, or am I wrong? https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:32: <li>{{javascript-method Use the <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Functions/Method_definitions">method definition</a> syntax where possible, even in preference to arrow functions.}}</li> On 2017/01/16 17:07:14, Sebastian Noack wrote: > I don't like how we overrule another rule here (i.e. "even in preference to > arrow functions"). Perhaps a better way to put it; "use the arrow function > syntax when passing anonymous functions" and "use the method definition > shorthand syntax when defining methods on an object". That way we have a clearer > distinction and mutually exclusive definition of the use cases, without need for > any exceptions. Done. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:32: <li>{{javascript-method Use the <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Functions/Method_definitions">method definition</a> syntax where possible, even in preference to arrow functions.}}</li> On 2017/01/16 17:07:14, Sebastian Noack wrote: > "method definition" is ambiguous as the old syntax (i.e. |{method: function() { > ... }}|) arguably is a form of method definition as well. However, we a re > specifically talking about the method definition _shorthand_ syntax here. Done. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:32: <li>{{javascript-method Use the <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Functions/Method_definitions">method definition</a> syntax where possible, even in preference to arrow functions.}}</li> On 2017/01/16 17:07:14, Sebastian Noack wrote: > "method definition" is ambiguous as the old syntax (i.e. |{method: function() { > ... }}|) arguably is a form of method definition as well. However, we a re > specifically talking about the method definition _shorthand_ syntax here. Done. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:33: <li>{{javascript-strict Where possible always use <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode">strict mode</a>.}}</li> On 2017/01/16 17:07:14, Sebastian Noack wrote: > Why the "where possible" restriction? Is there any legit use case for some > behavior that only exists outside of strict mode? Done. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:34: <li>{{javascript-if-else-braces When an <code>if</code>, <code>else</code> or <code>loop</code> statement spans over more than one line always enclose it with braces. When an <code>if</code> or <code>else</code> statement uses braces the other half should do too.}}</li> On 2017/01/16 17:07:14, Sebastian Noack wrote: > "other half" doesn't sound too technically accurrate, how about "opposing > block"? Done. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:34: <li>{{javascript-if-else-braces When an <code>if</code>, <code>else</code> or <code>loop</code> statement spans over more than one line always enclose it with braces. When an <code>if</code> or <code>else</code> statement uses braces the other half should do too.}}</li> On 2017/01/16 17:07:14, Sebastian Noack wrote: > "loop" isn't a literal code snippet, so it shouldn't use the <code> tag. Also > "loop" is not a statement (but a group of statements). > > > When an <code><fix>if</fix></code>, <code><fix>else</fix></code> or statement > or loop spans over multiple lines ... > Done. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:35: <li>{{javascript-for-of When iterating through Arrays and similar prefer <code>for (let item of array)</code> loops to traditional <code>for (let i=0; i < array.length; i++)</code> or even <code>array.forEach(...)</code> unless there is a good reason to do otherwise.}}</li> On 2017/01/16 17:07:14, Sebastian Noack wrote: > I think we are really only talking about arrays here, as there don't seem to be > any other objects which support both, length+indices as well as the iterator > protocol. > > Also I wonder whether we can come up with a less vague rule. The only "good > reasons" to not use for..of would be if you need to either access the the loop > index, or have to iterate in non-standard order (skipping the first/last, > reversing order, etc.), or if it isn't supported by any targeted browser. > > How about this: > > If supported, use for..of when iterating over arrays in canonical order, > rather than using for(;;) or the forEach() method. Done. https://codereview.adblockplus.org/29371951/diff/29371952/pages/coding-style.... pages/coding-style.html:36: <li>{{javascript-object-proto Use <code>Object.create(null)</code> instead of <code>{__proto__: null}</code> and otherwise avoid the use of <code>__proto__</code>.}}</li> On 2017/01/16 17:07:14, Sebastian Noack wrote: > Well, by now, you should rather use Map or Set objects if you need a hash table, > rather than using Object.create(null) to avoid potential collisions. Done. https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:27: <li>{{javascript-opening-braces Opening braces of object literals don't go on their own line when that would cause a syntax error.}}</li> (I noticed an extra word "in" in this rule.)
https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:31: <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> bind is not supported in IE8, websites does support IE8 and apparently we still support IE8 https://adblockplus.org/requirements#internet-explorer https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:32: <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> I think this applies only to where it's supported.
https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:29: <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> 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/... https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:30: <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> 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`. https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:31: <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> On 2017/01/17 12:26:05, saroyanm wrote: > bind is not supported in IE8, websites does support IE8 and apparently we still > support IE8 https://adblockplus.org/requirements#internet-explorer 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. https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:32: <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> On 2017/01/17 12:26:05, saroyanm wrote: > I think this applies only to where it's supported. Yeah, I think we should prefix all rules that require ES6+ features with "If supported, " like you already did for some of them. https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:33: <li>{{javascript-strict Always use <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Strict_mode">strict mode</a>.}}</li> 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. https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:35: <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> 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. https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:36: <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> 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.
Patch Set 3 : Addressed feedback, reordered rules added const rule https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:29: <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> On 2017/01/17 22:31:38, Sebastian Noack wrote: > 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/... Done. https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:30: <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> On 2017/01/17 22:31:38, Sebastian Noack wrote: > 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`. Done. https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:31: <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> On 2017/01/17 22:31:38, Sebastian Noack wrote: > On 2017/01/17 12:26:05, saroyanm wrote: > > bind is not supported in IE8, websites does support IE8 and apparently we > still > > support IE8 https://adblockplus.org/requirements#internet-explorer > > 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. I agree let's just remove this rule, it's not really relevant any more. https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:32: <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> On 2017/01/17 22:31:38, Sebastian Noack wrote: > On 2017/01/17 12:26:05, saroyanm wrote: > > I think this applies only to where it's supported. > > Yeah, I think we should prefix all rules that require ES6+ features > with "If supported, " like you already did for some of them. Good point Manvel, I moved all the modern JavaScript rules to their own section to avoid confusion. https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:33: <li>{{javascript-strict Always use <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Strict_mode">strict mode</a>.}}</li> On 2017/01/17 22:31:38, Sebastian Noack wrote: > 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. Yea good idea, I've done that but gone one better and created a new section for the modern rules. https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:35: <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> On 2017/01/17 22:31:38, Sebastian Noack wrote: > 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. Done. https://codereview.adblockplus.org/29371951/diff/29372129/pages/coding-style.... pages/coding-style.html:36: <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> On 2017/01/17 22:31:38, Sebastian Noack wrote: > 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. 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.
https://codereview.adblockplus.org/29371951/diff/29372265/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29371951/diff/29372265/pages/coding-style.... pages/coding-style.html:30: <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> Perhaps we should put the two rules about braces next to each other. https://codereview.adblockplus.org/29371951/diff/29372265/pages/coding-style.... pages/coding-style.html:31: <li>{{javascript-object-proto-null Use <code><fix>Object.create</fix></code> to create an object with a specific prototype rather than by specifying or modifying the <code><fix>__proto__</fix></code> property.}}</li> As it stands now, this rule is kinda redundant, as it is just a random case of "don't use deprecated APIs". Probably we don't need an explicit rule for that. https://codereview.adblockplus.org/29371951/diff/29372265/pages/coding-style.... pages/coding-style.html:38: <li>{{javascript-for-of When iterating over arrays in the canonical order use the new <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Statements/for...of">for..of</a> syntax if the loop index isn't required. (Rather than using <code><fix>for(;;)</fix></code> or the <code><fix>forEach()</fix></code> method.)}}</li> Just because "for..of" is linked now, doesn't mean that it cannot use source code formatting. Also don't forget <fix> to prevent it from being translated. <a href=".."><code><fix>for..of</fix></code></a> https://codereview.adblockplus.org/29371951/diff/29372265/pages/coding-style.... pages/coding-style.html:38: <li>{{javascript-for-of When iterating over arrays in the canonical order use the new <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Statements/for...of">for..of</a> syntax if the loop index isn't required. (Rather than using <code><fix>for(;;)</fix></code> or the <code><fix>forEach()</fix></code> method.)}}</li> Is it grammatically correct to end the sentence before the conjunction (i.e. rather)? https://codereview.adblockplus.org/29371951/diff/29372265/pages/coding-style.... pages/coding-style.html:39: <li>{{javascript-map-set Use the <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Map">Map</a> or <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Set">Set</a> data types rather than misusing plain objects.}}</li> I still think think talking about datatypes is inaccurate: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Data_... Also you no longer clarify what is considered misusing plain objects. I think the following would be more accurrate: Use the Map or Set objects, rather than misusing plain objects, when you need a hash table.
Patch Set 4 : Addressed feedback https://codereview.adblockplus.org/29371951/diff/29372265/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29371951/diff/29372265/pages/coding-style.... pages/coding-style.html:30: <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> On 2017/01/18 09:56:20, Sebastian Noack wrote: > Perhaps we should put the two rules about braces next to each other. Done. https://codereview.adblockplus.org/29371951/diff/29372265/pages/coding-style.... pages/coding-style.html:31: <li>{{javascript-object-proto-null Use <code><fix>Object.create</fix></code> to create an object with a specific prototype rather than by specifying or modifying the <code><fix>__proto__</fix></code> property.}}</li> On 2017/01/18 09:56:20, Sebastian Noack wrote: > As it stands now, this rule is kinda redundant, as it is just a random case of > "don't use deprecated APIs". Probably we don't need an explicit rule for that. Removed. https://codereview.adblockplus.org/29371951/diff/29372265/pages/coding-style.... pages/coding-style.html:38: <li>{{javascript-for-of When iterating over arrays in the canonical order use the new <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Statements/for...of">for..of</a> syntax if the loop index isn't required. (Rather than using <code><fix>for(;;)</fix></code> or the <code><fix>forEach()</fix></code> method.)}}</li> On 2017/01/18 09:56:20, Sebastian Noack wrote: > Just because "for..of" is linked now, doesn't mean that it cannot use source > code formatting. Also don't forget <fix> to prevent it from being translated. > > <a href=".."><code><fix>for..of</fix></code></a> Done. https://codereview.adblockplus.org/29371951/diff/29372265/pages/coding-style.... pages/coding-style.html:38: <li>{{javascript-for-of When iterating over arrays in the canonical order use the new <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Statements/for...of">for..of</a> syntax if the loop index isn't required. (Rather than using <code><fix>for(;;)</fix></code> or the <code><fix>forEach()</fix></code> method.)}}</li> On 2017/01/18 09:56:20, Sebastian Noack wrote: > Is it grammatically correct to end the sentence before the conjunction (i.e. > rather)? No idea, how about "As opposed ..."? https://codereview.adblockplus.org/29371951/diff/29372265/pages/coding-style.... pages/coding-style.html:39: <li>{{javascript-map-set Use the <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Map">Map</a> or <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Set">Set</a> data types rather than misusing plain objects.}}</li> On 2017/01/18 09:56:20, Sebastian Noack wrote: > I still think think talking about datatypes is inaccurate: > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Data_... > > Also you no longer clarify what is considered misusing plain objects. I think > the following would be more accurrate: > > Use the Map or Set objects, rather than misusing plain objects, > when you need a hash table. Done.
https://codereview.adblockplus.org/29371951/diff/29372555/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29371951/diff/29372555/pages/coding-style.... pages/coding-style.html:29: <li>{{javascript-opening-braces Opening braces of object literals don't go on their own line when that would cause a syntax error.}}</li> FWIW, I think the braces rules are slightly more important and should go above s18 as it was before. Just keep them together. https://codereview.adblockplus.org/29371951/diff/29372555/pages/coding-style.... pages/coding-style.html:37: <li>{{javascript-for-of When iterating over arrays in the canonical order use the new <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Statements/for...of"><code><fix>for..of</fix></code></a> syntax if the loop index isn't required. (As opposed to using <code><fix>for(;;)</fix></code> or the <code><fix>forEach()</fix></code> method.)}}</li> I think using "rather" was fine. In fact I think it's slightly better than "as opposed to". I was merely wondering whether the full stop belongs there, no strong opinion though.
Patch Set 5 : Addressed further feedback https://codereview.adblockplus.org/29371951/diff/29372555/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29371951/diff/29372555/pages/coding-style.... pages/coding-style.html:29: <li>{{javascript-opening-braces Opening braces of object literals don't go on their own line when that would cause a syntax error.}}</li> On 2017/01/18 10:54:31, Sebastian Noack wrote: > FWIW, I think the braces rules are slightly more important and should go above > s18 as it was before. Just keep them together. Done. https://codereview.adblockplus.org/29371951/diff/29372555/pages/coding-style.... pages/coding-style.html:37: <li>{{javascript-for-of When iterating over arrays in the canonical order use the new <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Statements/for...of"><code><fix>for..of</fix></code></a> syntax if the loop index isn't required. (As opposed to using <code><fix>for(;;)</fix></code> or the <code><fix>forEach()</fix></code> method.)}}</li> On 2017/01/18 10:54:31, Sebastian Noack wrote: > I think using "rather" was fine. In fact I think it's slightly better than > "as opposed to". I was merely wondering whether the full stop belongs there, > no strong opinion though. Done.
LGTM https://codereview.adblockplus.org/29371951/diff/29372557/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29371951/diff/29372557/pages/coding-style.... pages/coding-style.html:37: <li>{{javascript-for-of When iterating over arrays in the canonical order use the new <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Statements/for...of"><code><fix>for..of</fix></code></a> syntax if the loop index isn't required. (Rather than using <code><fix>for(;;)</fix></code> or the <code><fix>forEach()</fix></code> method.)}}</li> Detail: Please add full stop in the end of the line for consistency with other list items.
https://codereview.adblockplus.org/29371951/diff/29372557/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29371951/diff/29372557/pages/coding-style.... pages/coding-style.html:37: <li>{{javascript-for-of When iterating over arrays in the canonical order use the new <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Statements/for...of"><code><fix>for..of</fix></code></a> syntax if the loop index isn't required. (Rather than using <code><fix>for(;;)</fix></code> or the <code><fix>forEach()</fix></code> method.)}}</li> On 2017/01/18 11:27:02, saroyanm wrote: > Detail: Please add full stop in the end of the line for consistency with other > list items. Well, if you put the whole sentence into parentheses the full-stop belongs inside the parentheses as well. Alternatively, we could not end the sentence there (as I suggested before): ... isn't required (rather than using `for(;;)` or the `forEach` method). Or not even use parentheses: ... isn't required, rather than using `for(;;)` or the `forEach` method. Or we leave it as it is. No strong opinion.
https://codereview.adblockplus.org/29371951/diff/29372557/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29371951/diff/29372557/pages/coding-style.... pages/coding-style.html:37: <li>{{javascript-for-of When iterating over arrays in the canonical order use the new <a href="https://developer.mozilla.org/docs/Web/JavaScript/Reference/Statements/for...of"><code><fix>for..of</fix></code></a> syntax if the loop index isn't required. (Rather than using <code><fix>for(;;)</fix></code> or the <code><fix>forEach()</fix></code> method.)}}</li> On 2017/01/18 11:38:08, Sebastian Noack wrote: > On 2017/01/18 11:27:02, saroyanm wrote: > > Detail: Please add full stop in the end of the line for consistency with other > > list items. > > Well, if you put the whole sentence into parentheses the full-stop belongs > inside the parentheses as well. Learned something today. Fare enough. > Alternatively, we could not end the sentence > there (as I suggested before): > > ... isn't required (rather than using `for(;;)` or the `forEach` method). > > Or not even use parentheses: > > ... isn't required, rather than using `for(;;)` or the `forEach` method. > > Or we leave it as it is. No strong opinion. No strong opinion either. LGTM still from my side.
FWIW, I find the variants that don't end the sentence before the conjunction slightly nicer. But again, no strong opinion. So LGTM. 
|
