Topic on Manual talk:Coding conventions/JavaScript

JGonera (WMF) (talkcontribs)

I noticed something in the mobile extension code and discussed it with Jon (although I'm not sure if I convinced him).

In the existing code a DOM element from a jQuery object is often passed instead of simply passing the jQuery object, e.g.:

setupReferences( $( '#content' )[ 0 ] );

Jon said that he wants to be sure that only one element is passed. I think it just makes the code look more confusing and doesn't really solve the problem. I mean we should simply assume that the selector selects only one element. If it selects more than one element it's obviously a mistake and we actually can't say if [0] is the right one or maybe [1] or [54]. If we really want to be sure we can check .length at the beginning of the invoked function.

Anyone disagrees? Could we include that somewhere in the coding conventions?

Krinkle (talkcontribs)

ID selectors are incapable of returning more than one element. So if you're referring specifically to code using [0] on an object from an ID selector, that doesn't deserve mention in code conventions as it is imho just plain stupid. Something you might be confused by once, but if we start including things like that there is no telling where it will end end. Code conventions isn't a training guide to how the browser works and how the DOM works.

For passing around 1 element, you can use .eq( 0 ) instead of [0] to yield a cloned jQuery object that only contains the first node of the jQuery object collection.

Regarding passing HTMLElement objects or jQuery collections between functions, it depends on what makes sense for the situation. I don't think this is the kind of thing where there is 1 right solution or where using different approaches in different modules makes code harder to use.

jQuery plugins obviously operate on a jQuery collection as they are methods on the jQuery prototype itself. Utilities often operate on 1 single element. In the latter case it is imho preferred to document the method as taking an HTMLElement as opposed to a "jQuery object that must have only 1 element". That way the responsibility of extracting the right data is in the hands of the caller. So that you have Foo.bar( $foo[0] ) instead of Foo.bar( $foo ). That way Foo.bar doesn't have to verify it has length, take the first one, and maybe throw an error if it has more than one?

Also note that constructing a jQuery object for a node is very cheap. It is the first executive use case in the constructor. Nothing to worry about there. Things like $( this ) in DOM callbacks are very common and very fast.

So something like this would be fine:

/** @param {HTMLElement} el */
Foo.getState = function (el) {
  return $(el).data('foo-state');
};

However if utilities are more efficient when they can cache data outside a loop, then you may want to change the function to take an array-like object instead of requiring the callee to make a loop where Foo.bar then has to do initialisation for every single call inside the loop.

So, instead of this:

/** @param {HTMLElement} el */
Foo.setStuff = function (el, stuff) {
  var state = Foo.getState(el) || {};
  state.stuff = Foo.normaliseStuff(stuff);
  return $(el).data('foo-state', state);
};

// One
Foo.setStuff($('#foo-container')[0], /* .. */);

// Many
$('.foo-items').each(function () {
    Foo.setStuff(this, /* .. */);
});

The following would likely be better (so that stuff is only normalised once):

/** @param {Array|jQuery} els */
Foo.setStuff = function (els, stuff) {
  var state, i, len;
  stuff = Foo.normaliseStuff(stuff);
  for (i = 0, len = els.length; i < len; i++) {
    state = Foo.getState(el) || {};
    state.stuff = stuff;
    $(els[i]).data('foo-state', state);
  }
};

// One
Foo.setStuff($('#foo-container'), /* .. */);
// Many
Foo.setStuff($('.foo-items'), /* .. */);

This sort of a bad example since jQuery methods support setting on the entire collection at once, so this is probably better done like $('.foo-items').data('foo-state-stuff, /* .. */); but you get the idea. Also, you could extend Foo.setStuff with els = els.nodeType ? [ els ] : els; so that it supports both:

/** @param {Array|jQuery|HTMLElement} els */
Foo.setStuff = function (els, stuff) {
  var state, i, len;
  els = els.nodeType ? [ els ] : els;
  stuff = Foo.normaliseStuff(stuff);
  for (i = 0, len = els.length; i < len; i++) {
    state = Foo.getState(el) || {};
    state.stuff = stuff;
    $(els[i]).data('foo-state', state);
  }
};

// Alternative:

/** @param {Array|jQuery|HTMLElement} els */
Foo.setStuff = function (els, stuff) {
  $(els).data('foo-state-stuff', stuff);
};

// Alternative (2):

/** @param {jQuery} $els */
Foo.setStuff = function ($els, stuff) {
  $els.data('foo-state-stuff', stuff);
};


// One (node)
Foo.setStuff($('#foo-container')[0], /* .. */);
Foo.setStuff(document.getElementById('foo-container'), /* .. */);
// One
Foo.setStuff($('#foo-container'), /* .. */);
// Many
Foo.setStuff($('.foo-items'), /* .. */);
Wikinaut (talkcontribs)

@Krinkle

(I suggest that) perhaps a shortened version of your reply should go to the article page.

JGonera (WMF) (talkcontribs)

Thanks for the explanation. Maybe I sounded stupid bringing up a problem like that. It just bothered me to see $(something)[0] all around the code and I needed some arguments to convince Jon to stop doing that :)