[policy, no patch] Update JS coding standards

RE idea #1 - I am not seeing the difference between the current standard and what you are proposing? They look the same to me.

RE the rest -- this is referring to what page and what paragraph? It would be a lot easier to understand what you are proposing if you put "before" and "after" sections from the page so we could compare.

Oh and you forgot the tag...

Event namespaces

1. Each bound event should be namespaced to the module (for modules) or the core file (for misc files) that implements it.
2. The namespace should match the module or file name, module being preferred.
3. Namespaces are lowercase and single words. No hypens, underscores or camelCasing.
4. Additional namespaces can be added after the primary module/file namespace e.g. click.toolbar[.anothernamespace] where additional namespaces provide an end user access to alter core Drupal functionality

Examples

click.toolbar
mouseenter.tabledrag

Data namespaces

referring to the jQuery.data() method.

1. Each data method invocation should be namespaced to the module (for modules) or the core file (for misc files) that calls it.
2. The namespace should match the module or file name, module being preferred.
3. Namespaces are lowercase and single words. No hypens, underscores or camelCasing.

Examples

$('div').data('toolbar', {});
$('div').data('tabledrag', {});

Can someone please update the issue summary with a list of:
a) What needs to be deleted from the current JS coding standards page.
b) What needs to be added.
c) What needs to be changed (before/after text would be helpful).

Otherwise this issue cannot really be reviewed. Thanks!

I don't like the beginning of the new proposed text at all. That UL list is just ... a few hints or something? Really what is needed at the top of that page is a table of contents with #links in it so people can actually find things.

Also, the new text is written with "should" in places... does that mean things that you have to do or things that you can do if you want to? It would be better to use more precise language, such as "must", or else explain what the exceptions are.

Also, we don't normally keep different standards pages for different versions of Drupal. We just don't go back and update previous versions of Drupal to comply with standards adopted after they were released.

Coding standards pages are generally about code writing style (indentation, choices on how to capitalize stuff, etc.), not about which functions to use where. That information should probably be elsewhere rather than in the coding standards section, and the coding standards should not be version dependent. They should be about best practices for any version of Drupal, and should not cause problems if they are followed in a previous version of Drupal. Check the rest of the coding standards pages and I think you'll find that is the case.

Just to clarify... In the Coding Standards section, we want to have style standards that are generally applicable. If you have programming conventions like "In Drupal 7, we do things this way, and in Drupal 8, we do things this way", the best place to put them would be somewhere in:
http://drupal.org/developing/api

FWIW, I updated the jQuery coding standards page (https://drupal.org/node/1720586). Not sure if there is actually an issue for that, so posting here.

up issue sum

Updated the issue summary to list out the standards changes, and the remaining tasks. Removing the needs issue summary update tag.
---

A.
Since we wont have a different standard for drupal 8 from others, are all these changes really wanted? Updating 6 or 7 to them might be more than we want to do.

---

I went through the diff from #7 and it seems these are the changes to the standard (the other things are changes to wording or formatting of the text of the standards doc page.

[edit: ug. used [code] and [/code] instead of < and > cause embedding code inside of code made it strange when viewing.]

1. change to function declaration spacing standard.

   change is to always have a space before the (

   +++ ../PATCHS/js-standards.txt	2013-03-28 12:24:42.129478625 +0100
   @@ -149,15 +194,14 @@
   -function funStuff(field, settings) {
   +function funStuff (field, settings) {
      settings = settings || Drupal.settings;
      alert("This JS file does fun message popups.");
      return field;
    }
    [/code];
    <ul>
   -<li>There should be no space between the function name and the following left parenthesis.</li>
   -<li>Exception: If a function literal is anonymous, there should be a single space between the word "function" and the left parenthesis "(". Otherwise, it can appear that the function's name is actually "function".</li>
   +<li>There is always a space between the function declaration and the following left parenthesis.</li>
    <li>Define optional arguments (using default values) at the end of the function signature.</li>
    <li>Always attempt to return a meaningful value from a function if one is appropriate.</li>
    </ul>
   

   Why this change? Is this something other projects do? Is there a industry recommendation for this?
   It seems opposing to the jQuery standard: https://drupal.org/node/1720586
   https://github.com/stevekwan/best-practices/blob/master/javascript/best-... does not use the space before ( for functions
   and neither do the examples in http://www.w3.org/wiki/JavaScript_best_practices#Stick_to_a_strict_codin...
   I'm not sure where to look, since I'm not familiar with other projects or javascript.

   and the drupal php standard:
   https://drupal.org/coding-standards#functdecl

   for some examples of php anonymous functions
   $ ag "\->will" core/modules | grep function

   and

   $ ag "function[(]" core/modules | grep php | wc -l
         60
   $ ag "function [(]" core/modules | grep php | wc -l
         63
   

2. change to variable assignment standard.

   take out allowing white space to align assignment values.

   +++ ../PATCHS/js-standards.txt	2013-03-28 12:24:42.129478625 +0100
   @@ -169,16 +213,12 @@
   -As displayed above, there should be one space on either side of an equals sign used to assign the return value of a function to a variable. In the case of a block of related assignments, more space may be inserted to promote readability:
   -[code]
   -short        = foo(bar);
   -longVariable = foo(baz);
   -[/code]
   +
   

   This takes out the section about allowing it, but this does not, not allow that. Maybe it should say explicitly that assignments must have only one space on either side of = ?

   I didn't know this, but looks like https://drupal.org/coding-standards#functdecl
   allows that white space to align assignments (for function calls??),
   but that seems contradictory to the section about operators: https://drupal.org/coding-standards#operators

3. change to with statement.

   says not to use with, and to use (the shorthand) explicit version

   +++ ../PATCHS/js-standards.txt	2013-03-28 12:24:42.129478625 +0100
   @@ -226,23 +266,10 @@
    <h2 id="with">"with" statement</h2>
   -The [code]with[/code] statement was intended to provide a shorthand for accessing members in deeply nested objects. For example, it is possible to use the following shorthand (but not recommended) to access [code]foo.bar.foobar.abc[/code], etc:
   -[code]
   -with (foo.bar.foobar) {
   -  var abc = true;
   -  var xyz = true;
   -}
   -[/code]
    
   -However it's impossible to know by looking at the above code which [code]abc[/code] and [code]xyz[/code] will get modified. Does [code]foo.bar.foobar[/code] get modified? Or is it the global variables [code]abc[/code] and [code]xyz[/code]?
   +Do not use the [code]with[/code] statement.
    
   -Instead you should use the explicit longer version:
   -[code]
   -foo.bar.foobar.abc = true;
   -foo.bar.foobar.xyz = true;
   -[/code]
   -
   -or if you really want to use a shorthand, use the following alternative method: 
   +Instead you should use the explicit version:
    [code]
      var o = foo.bar.foobar;
      o.abc = true;
   

4. change to comparison operators allowed.

   +++ ../PATCHS/js-standards.txt	2013-03-28 12:24:42.129478625 +0100
   @@ -252,6 +279,8 @@
    <h2 id="operators">Operators</h2>
    <h3 id="truefalse">True or false comparisons</h3>
   +Always use strict comparaison using [code]===[/code] and [code]!==[/code].
   +
    The [code]==[/code] and [code]!=[/code] operators do type coercion before comparing. This is bad because it causes:
    [code]' \t\r\n' == 0[/code]
    to be true. This can mask type errors. When comparing to any of the following values, use the [code]===[/code] or [code]!==[/code] operators, which do not do type coercion:
   
   @@ -301,30 +330,60 @@
    <h2 id="typeof">Typeof</h2>
    When using a [code]typeof[/code] check, don't use the parenthesis for the typeof. The following is the correct coding standard:
    [code]
   -if (typeof myVariable == 'string') {
   +if (typeof myVariable === 'string') {
   

   is this a standard change or a code implementation recommendation? maybe this should be moved to the api docs jhodgdon mentioned in #12

5. Adding to the standard that js files should have a closure.

   +++ ../PATCHS/js-standards.txt	2013-03-28 12:24:42.129478625 +0100
   @@ -301,30 +330,60 @@
   +<h2 id="closure">File closure</h2>
   +All js files should have a closure to avoid adding variables to the global namespace. The parameter of the closure correspond to the dependencies needed for the script to work. If a script declare jQuery, Drupal, drupalSettings and underscore as dependencies in [code]hook_library_info()[/code] as so:
   +
   +[code]<?php
   ...
   

grammar fix and adding which comment issue summary is update as of.

RE #15 - I think I agree with all the points YesCT raised. We need a new patch.

Are #3 and #4 suggested *additions* to the js standards?

I think each change should have it's own issue so we can discuss them separately.

Maybe there can be a general wording clean up issue (which also adds table of contents) also for the changes in #7 that are not actual standards changes. So we can more easily see really what is being suggested.

Agree on function () and function something() spacing standard. Fine, I don't care that much.

I do happen to care about this one :) Mostly because I don't want to introduce a very subtle distinction into the standards like this. I'd rather the rule be uniform:

"When declaring a function, leave a space before the parenthesis; when invoking a function, no space". So

function () {}
function something () {}

RE #20/21 - If leaving a space between the function declaration and the () is standard in the industry for JavaScript, let's also adopt that standard. If it's not standard, we should not adopt it here either. Can either of you provide something beyond "I care about it" and "I like it" to back this up? Do IDEs have this as their standard when declaring functions in JS? Do other projects with actual standards do it this way? Do code prettifiers have this as their default?

Because... As someone who only dabbles in JavaScript occasionally, it sure looks odd to me (especially as someone who is used to the PHP standards we have in the Drupal project). Also, what if there are function arguments between the ()?

And... Please, let's have the discussion, rather than giving up before it's hardly started (in spite of this being open for a while, there hasn't been much actual substantive discussion about the standards themselves).

And regarding #18, I think someone could do a quick edit/cleanup/add TOC pass through the standards document -- agreeing not to change any of the substance of the page -- and post a link to the revision so we could verify. Then we could use that as the starting point for proposing new standards? I don't think we need an issue for "clean up grammar, formatting, etc.", just for "this standard needs revision".

RE #20/21 - If leaving a space between the function declaration and the () is standard in the industry for JavaScript, let's also adopt that standard. If it's not standard, we should not adopt it here either. Can either of you provide something beyond "I care about it" and "I like it" to back this up? Do IDEs have this as their standard when declaring functions in JS? Do other projects with actual standards do it this way? Do code prettifiers have this as their default?

Sublime Text 2 auto-expands to this:

function function_name (argument) {
  // body...
}

jQuery's JavaScript guidelines are just ugly. They're full of whitespace where it's not needed and lacking where it is. Plus every one of their rules (hyperbole but close) has a list of exceptions. I'd like to see us strive for consistency that doesn't require deep knowledge of the syntax -- just simple patterns.

I quite like our existing JSDocs. I'm looking out to code reviews where we're going to have to write: "Well, yes, you need a space between function and ( normally, but because you named your function, now you don't need a space"...over and over again. Esoteric exceptions just create friction.

+1 for consistency between named and unnamed functions' whitespace, and +1 for adopting things that IDEs our developers are actually using do already by default.

And +1 for not adopting guidelines from JQuery if they're lame. If there's another set of generic JS guidelines out there we can crib, great; otherwise, let's make our own that are at least simple to understand and follow, and as consistent as possible.

So, the current standards section on functions ... dang, hard to link to. I'm going to go edit that page and add a TOC, and if I have time in my day today, I'll come back and make some more comments here. But the arguments in #23 compared to what we have now do indeed look compelling. Thanks!

OK, I just edited https://drupal.org/node/172169 and added a table of contents at the top... which really highlights the mix of topics there. Some are about the code formatting; others are about which JS built-ins to use and not use; others are about how to interact with Drupal. The next step would probably be to reorganize the page a bit more logically, and move anything there that is version-specific to a different page... but that's not really what we're discussing here.

Meanwhile, let's see. YesCT did a nice job of pointing out which items in the current standards were proposed for revising in the last patch.

The one we are discussing in the past few comments, and the only one there seems to be much disagreement about, is for declaring functions. The current standard is:
https://drupal.org/node/172169#functions

See if this is a fair assessment:
- The existing standards are inconsistent in how they are doing things in this section.
- Both jessebeach and nod_ (with some backing by IDEs) think that we should adopt a standard that results in code like this for declaring functions:

// (a) Anonymous literal with params assigned to a variable:
Drupal.behaviors.tableDrag = function (context) {
...
// (b) Named function with params:
function funStuff (field, settings) {
...
// (c) Anonymous literal with no params assigned to a varialble:
foo = function () {
...

(a) is what is already in our standards doc; (b) is not, but is consistent with (a).
- The existing standards say, in words:

• There should be no space between the function name and the following left parenthesis.
• Exception: If a function literal is anonymous, there should be a single space between the word "function" and the left parenthesis "(". Otherwise, it can appear that the function's name is actually "function".

The proposal is to make this much simpler, something like this:

When declaring a function, there should be one space between the keyword "function" or (if it is a named function) the function name and the opening left paren "(".

Right?

To me, this seems consistent, and apparently at least one IDE agrees.

It should be also more consistency for other world parties as possible as it can. Not every contributors hired by Drupal companies and only worked for Drupal. They won't read the Code Standards until one day you're telling him: You do something WRONG!!

Can you show 3 popular IDEs example & JS libs promoted this rule. Thinking in another side, while Sublime Text do this auto-expand, why no scripts adopt it? Why guys spending more time to remove it's quick-expand styles.

No SPACE++ ( #4 ~ #6, https://drupal.org/comment/8593879#comment-8593879 )

Don't forget the Named function expression. :)

var foo = function foo() { }

** Also don't create new rules for every Drupal version. :(

@mparker17 just posted this in IRC:

I don’t know if this would help, but have you seen https://github.com/rwaldron/idiomatic.js/ or http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml … those might be a good starting point for coding standards which are consistent with other projects. (see also http://jstherightway.org/ )

Would be worth looking into.

Should we update the lints also?
Or maybe lint is just for syntax errors?

If this pertains only to the changes outlined in #32, I don't have any objections. It might be nice to have some more elaboration on strict mode, but that's an extra "feature", not a bug.

The summary outlines a lot more though.

@33: Most of these will be caught by JSHint by default, especially since we enforce strict mode through JSHint. But linting is generally to avoid silly errors and things that are easily misinterpreted, coding standards are more about readability and maintainability.

One day, I will get the hang of these here dropdown widgets...

Agreed to the "agreed-to"'s in the issue summary. Let's do this.

Can someone please update the issue summary to summarize what policy has been or is being agreed to on this issue? It is hard to review when the issue summary is not updated.

Such as, it says "Agree to ... standard" but it doesn't say what that standard is, where to find the updated page in the summary, or if it's just an edit of the existing page, what revision diff we should be looking at. Thanks!

In general, this looks good to me.

Implanted IETF/RFC keyword terminology: https://drupal.org/node/172169/revisions/view/7255765/7271425

Summary of my review:

1. A high-level concern from my side is that the document mixes coding style standards with best practices in various locations. The most prominent example is the chapter about eval(), which has nothing to do with code style/formatting.

   This causes the scope and purpose of the document to get fuzzy. Based on my experience with coding standards, such mixing tends to cause confusion for future discussions and updates, because people (reasonably) cannot understand what parts of the document are about formatting and which are about "recommended practice for authoring JS code in Drupal".

   The IETF/RFC terminology helps to mitigate that by making clear what's required and what isn't (MUST vs. SHOULD vs. RECOMMENDED vs. MAY), but it does not help to clarify the scope.

   I would recommend to split the best practices into a separate document.

2. The document additionally contains multiple explanations of basic JavaScript language constructs. Those are definitely off-topic for a coding style/formatting document. And even in a separate best practices document, I would still not understand why we're trying to educate about JavaScript essentials on our own...

   The most prominent example is the note that Booleans must be written in lowercase in JavaScript, because uppercase… — Such documentation does not make sense, because it documents the JavaScript language itself. If you write invalid JavaScript code, your code blows up. End of story.

   I would recommend to remove those parts altogether without replacement.

3. Some more granular items:

   1. The strict mode chapter does not clarify whether "use strict"; should be added to every function, or whether a single declaration per file/closure is sufficient.
   2. The chapter "Constants and Global Variables" does not document anything about global variables, only the preceding "Variables and Arrays" chapter does.

   3. Pre-defined constants SHOULD use lowerCamelCasing.

      The meaning of "constants" could be clarified, because there is/was no such thing until very recently.

      ECMAScript 6 introduced the const keyword, but it isn't supported on all browsers yet.

      Regardless of the usage aspect, the proposed standard of using lowerCamelCase for constants comes very unexpected. In the vast majority of programming languages, constants are declared as UPPER_UNDERSCORED.

      Especially when writing backwards-compatible code, you have to use var instead of const, and the common norm is that an all-uppercase variable name denotes that its value is supposed to be read-only.

      I would recommend to adjust the standard for constant names to say that they SHOULD be in all-uppercase, words separated by underscores.

   4. The chapter about "Typeof" needs revision. Not even I was able to make sense of the intended meaning, or what exactly it tries to say/rule.

      In type comparisons, the value to test SHOULD NOT use parenthesis.

      I expected it to talk about the comparison value (e.g., === 'undefined' vs. == undefined), but it only talks about the value/expression that is being tested — which makes little to no sense, because if you wrap the value to be tested into a string, then its type is always a string.

   5. Multiple related assignments MAY use additional spaces, if that aids in readability.

      I've personally been a fan of doing that back in 2006. But in the meantime, I've experienced way too many patches that had to touch a dozen of unrelated lines, just to adjust the vertical alignment of variable assignments, in case a new variable had a longer name, or in case the longest variable name was removed or shortened.

      That's not only cumbersome for developers — worse, it additionally tampers git's native content change tracking + pickaxe facility with unnecessary changes. If you pickaxe for a variable name that is vertically aligned, then you commonly get a completely useless resulting list of commits that only added or removed some spaces on the declaring line. Coding standards SHOULD NOT work against useful history parsing tools.

      I would recommend to either drop this statement, or perhaps even reverse it into "SHOULD NOT be vertically aligned".

   6. The "Constructors" chapter could use an example.

   7. The "Comments" chapter uses weird terminology:

      Non-documentation comments are strongly RECOMMENDED.

      All code comments are documentation, so the terminology used is illogical.

      I assume that "non-documentation" tries to refer to JSDoc blocks…? If that is the intended meaning, then we should replace those instance with the proper terminology.

      That said, only a small part of this chapter is within the scope of coding style/formatting. The parts about how & why should be moved to the separate + preexisting document about JavaScript documentation.

I generally agree with sun's statement sin #40.1 and #40.2.

I think to help all of us understand which are "rules" (MUST [NOT]), which are "standards" (SHOULD [NOT]) and which are "best practices" ([NOT] RECOMMENDED) we should prefix lines appropriately (see below). We can then worry about splitting things up accordingly later.

FYI, anything in blockquotes is my "rewrite" (doing it here so we can discuss/approve before making changes). I'm only doing the first few sections (mainly due to time) but also to see if this is the format we'd like to present (which I think is much clearer). Here's a link to this comment's HTML if anyone wants to update the page with what I've done: http://privatepaste.com/0a717dc5fe

I'll have to go through the rest of the sections when I get more time.

Whitespace (formerly "Indenting")

I also think the Function Calls (under Functions) should be moved to this section as they deal entirely with whitespace.

RULE:

• All code MUST indent using two (2) space characters.
• All code MUST NOT indent using tab characters.
• All code MUST NOT end with trailing whitespace.

Semi-colons

JavaScript allows any expression to be used as a statement and uses semi-colons to mark the end of a statement. However, it attempts to make this optional with "semi-colon insertion"; using this feature can mask errors and can also cause JS aggregation to fail if there are any missing semi-colons.

RULE:

• All statements MUST start on a new line.
• All statements MUST end with a semi-colon (;).
• Return values MUST start on the same line as the return keyword.

EXCEPTIONS:
Control structures (for, function, if, switch, try, while) that use curly brackets ({}). This DOES NOT include functions assigned to variables or objects like:

Drupal.behaviors.tableSelect = function (context) {
  // Statements...
};

• Lines ending (last character) in opening curly bracket ({).
• Lines ending (last character) in a closing curly bracket (}).

Closures/Strict mode

I think these two can be combined really. Also there appears to be nothing about closures in general on the page. I'm not entirely sure how we want to go about this. Leaving for discussion.

Coding standards changes are now part of the TWG

For now I added a note on the main coding standards page https://www.drupal.org/node/172169/revisions/view/8038301/8400517 since it was pretty buried and non-obvious.

What's the code standard for data-* attributes ? Can we skip `drupal` namespace ?

Getting a little meta, but how does the process being defined in #2428153: Create and document a process for updating coding standards affect this proposal?

What is left to do here. Can someone update the issue summary?

I'd suggest to fork this page as our standard:
https://github.com/airbnb/javascript/tree/master/es5

- this is about 99.8% of what we needed and even better.
- we just need to update a bit `Whitespace` rule in function
- add back for-in suggestions

Moving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.

@droplet, why not do the fork and make the change so we can review? I skimmed the page it looks good.

Check it out:
https://github.com/KayLeung/javascript/tree/master/es5

Compare it to Airbnb master:
https://github.com/airbnb/javascript/compare/master...KayLeung:master

Pull requests are welcomed!

Um... why are we forking airbnb? @nod_ has already gone through all of this (see issue summary and related issues). AFAIK, there's not really left to do... but I could be mistaken.

Don't make something never-end :)
We have to introduce great resource into Drupal and solve the problem. Same to this idea: #2289619: Add a new framework base theme to Drupal core

It seems a bit late in D8 JS now, but if we looking at other style guide, all has no spaces in function expressions:
https://github.com/jscs-dev/node-jscs/tree/master/presets

// bad.
function () {
}

// good.
function() {
}

So what are the JS Coding standards? The official doc pages are marked out of date with a link to this issue. From reading this issue all I see is an ES lint definition and a (possibly) good js doc standards page that was forked from airbnb.

What are the current standards?

ES6 is not so far there. It's coming tomorrow. Just today we see WebKit hit 100% ( https://news.ycombinator.com/item?id=11708840 ). Coding Guide should be defined before it's come true I think. Forking airbnb, we will save another 4 years for the wording.

For next 4 years, we should try to do this: https://angular.io/docs/ts/latest/guide/style-guide.html
A guide for better extending of JS code in Drupal.

Personally, I think we would do better to have our official coding standards on drupal.org. My question is more around what the coding standards are.

Do we have one? Should I just copy everything from the Airbnb fork over to d.o? I can update the d.o doc page. I just want to know what to take the standards from.

I checked all comments in this issue, and all proposed issues have been addressed and are published. The last on topic comment was in #46 but this has also been addressed in the meanwhile in #1337022: [policy, no patch] Create/adopt JavaScript docs standards compatible with a JS documentation parser.

Starting from comment #47 we had a couple of specific suggestions but these are out of scope for this issue. It seems like these suggestions were posted here mainly because the JS coding standards page currently links to this issue.

In particular there has been a suggestion from @droplet to replace our entire JS coding standards by the one from Airbnb, but this is out of scope for this issue. The goal was not about throwing out our current coding standard and rewriting it from scratch, but simply to clean up the coding standards page and bring it up to date with the current standard that emerged in core over the past years. If desired a new issue can be opened to discuss this, but it seems to me that this would be considered too disruptive to be accepted by the community.

I compared the coding standards page with the version from 2012 and it has vastly improved. In fact there are only few paragraphs left that have not been touched in the lifetime of this issue. Overall it looks pretty good now.

There doesn't seem anything left to do here. There will always be small things to improve, but we can tackle these in separate followup issues.

Removing the "work in progress" notice from the documentation page and marking this issue as fixed. Please feel free to open followup issues to propose specific improvements.

We should not close the issue in this way :)

the adoption from Airbnb (presentation) implies we can adopt the ES2005+ to Drupal also. I don't think we should waste any time on a new issue.

Handling that discussion in a dedicated issue allows for a more efficient discussion, where participants don't need to read and sort through fifty comments, some up to four years old, in order to determine the actual context for the discussion.

Also, while the ES5 proposal is to update the our coding standards *policies*, the spirit of this issue was to update our coding standards *documentation*; which is a different scope.