Javascript conventions

[phamann]

Below are some minor observations gathered during refactoring our JS bootstraping and modules. Some of which are consistent and others which need to be decided upon and then normalised across the modules and we can create a document or github page by which we can reference for future development.

Discussion on these conventions is encouraged ;) and please feel free to add more to the list!

Module initialising:

• If module requires multiple parameter/arguments to initialise, a single object with named keys is prefered i.e:

new AutoUpdate({
     path: window.location.pathname,
     delay: 10000,
     attachTo: qwery(".matches-container")[0],
     switches: switches
}).init();

over

new AutoUpdate(indow.location.pathname, 10000, qwery(".matches-container")[0], switches);

• All methods of a modules should be private unless methods that are required externally such as init(). *Some of are modules return classes some of them don't. A modules should only be called using the 'new' constructor if there needs to be multiple instances on the page such tabs.

  Naming conventions:

• All file and method names should be camelCased with no hyphens or underscores.
• Shorter abbreviated names are prefered for common actions, such as: init over initialise
• Show and Hide action names are preferred if manipulating the DOM such as: showFrontFixtures();
• Load is preferred over transclude if module is handling ajax requests.

  Events:

This is a controversial one:

• EventEmitter pub/sub is expensive, and makes modules harder to read. Therefore, a module should only emit and event if it is required for cross app/module communication or testing, and not for internal communication.
• EventEmitter itself even warns if we go over a certain amount of events globally.

  Logging:

• All defensive error situation should be logged using the error module:

common.mediator('module:error', 'Failed to load more matches', 'more-matches.js');

[commuterjoy]

Multi-line your define statement as it's easier to diff,

define([
    'common',
    'reqwest',
    ...
], function (
    common,
    reqwest,
    ...
) {

[commuterjoy]

A modules should only be called using the 'new' constructor if there needs to be multiple instances on the page such tabs.

How about just one module pattern.

Given the object based one doesn't support multiple instances can we use this [below] for everything?

function Foo(opts) {  // --- pass options in to constructor, not init()  

   // variable declaration etc.
   var options = opt,
        foo = options || false;

   // code to do with dom manipulation and user interaction goes in here
   this.view = {
      x: function() {  }
   } 

   // code to manipulate the data etc. 
   this.x = { };
   this.y = { };      

   // bindings
   common.mediator.on('module:xxx', this.view.x);

   // initialise
   this.init = function() { };

   return Foo;
}

[commuterjoy]

Iterators are easier to read than for loops,

Boo!

for (var i = 0, j = list.length; i<j; ++i) {
   document.cookie = list[i] + "=;path=/;expires=Thu, 01 Jan 1970 00:00:01 GMT;";
}

Yay!

list.forEach(function(item) { 
   document.cookie = item + "=;path=/;expires=Thu, 01 Jan 1970 00:00:01 GMT;";
});

Boo!

var foo = [1,2,3], query = '';
for (var k = 0, l = foo.length; k<l; ++k) {
  query += "a=" + foo[k] + "&";
}

Yay!

var query = [1,2,3].map(function(i){
 return "a=" + i;
}).join('&')

[commuterjoy]

Using a docblock will get you shot, apparently.

Though it's quite useful when it comes to opts style arguments,

/*
 @param {Object} options hash of configuration options:
   prependTo   : {DOMElement}  DOM element to prepend component to
   competitions: {Array}       Ordered list of competetions to query
   path        : {String}      Used to overide endpoint path
   contextual  : {Boolean}     Whether or not component links should be contextual
   numVisible  : {Number}  Number of items to show when contracted
 */

[mattandrews]

The forEach() syntax ties us to IE9+, which is my only concern, but then maybe we don't care about this...

[phamann]

This one line, is currently stopping JS getting to < IE9 browsers anyway.

Although - looking at that - the only part that returns false is the addEventListener, and Bean does a good normalising events and falling back to attachEvent instead. So we may want to remove. Thoughts?

But yes forEach() and map() will definitely fix us to IE9+

[commuterjoy]

Emissions,

emit("modules:name:event", [param1, param_2])  // Nb. use an array for any parameters

Logs errors,

emit('module:error', 'message', 'file.js');

Sometimes, when your code is dealing with multiple instances it might make sense to include the id and state in the emission,

common.mediator.emit('modules:name:event:' + id + ':' + state);

I'm not 100% sure this is a good thing.

[commuterjoy]

Lowercase all modules dependencies (even vendor ones) and name them after the module, camel-cased if necessary,

define(['common', 'modules/detect','modules/adverts/audience-science'], function (common, detect, audienceScience) {

DH - Feels odd saying new audienceScience(). I think it's useful to differentiate between objects and classes, e.g. camelCase for qwery, CamelCase for AudienceScience

[commuterjoy]

Don't use underscores to denote variable/method privacy.

[commuterjoy]

Always allow document roots to be injected.

It's easier to write tests for, and lets us scope the selector to a small part of the DOM, which is better for performance.

Boo!

var f = function() {
   document.getElementById('foo')
   }

Yay!

var f = function(opts) {
   var doc = opts.document || window.document; 
   doc.getElementById('foo');
   }

[commuterjoy]

Boo!

var foo = 1;
var bar = 2;

Yay!

var foo = 1,
    bar = 2;

Hipster!

var foo = 1
  , bar = 2;

[commuterjoy]

Misc.

• Module dependencies
• IE8
• Failure states pattern
• Feature toggle pattern
• (semi)-persistance pattern

[ironsidevsquincy]

jshint picks up on multiple var declarations, right? Doesn't have a isHipster setting though

[commuterjoy]

RE: Jasmine spec's

Must have a corresponding test named after the module.

Programmatically create fixture HTML using the fixtures helper

Don't chain your tests. A new instance for each test of whatever you are testing is clearer.

waitsFor is more obvious than waits

Keep everything inside beforeEach (otherwise you risk tests bleeding in to each other)

Use sinon spying over jasmine (it's got a better feature-set)

Clear the mediator listeners before all tests.

Think about why you are using waits(500) - would the test be clearer if you linearised the logic?

Reading globals - Eg, location, document - via injection Vs. common amd module vs read directly?

[mattandrews]

Have moved this discussion into a dedicated page (https://github.com/guardian/frontend/wiki/JavaScript-guidelines) -- closing this, let's move discussion over there instead!