Signs of a poorly written jQuery plugin

So far with every single workshop I’ve given, both for advanced JavaScript and jQuery for Designers, this question (or some variation thereof) has come up:

How do you know if the plugin is good to use?

It’s always dependant on the problem they’re trying to solve, but in lieu of a better jQuery plugin ranking system, here’s a couple of tips that should raise a red flag.

Consider the following:

$.fn.myplugin = function () {
  var me = $(this).each(function() {
    return $(this).bind('someEvent', function () {
      // does something
    });
  });

  return me;
};

Although the code may be perfect once some event has run, most times you don’t have time to read through all the code carefully and you need to make a decision so you can move on to the actual problem you’re trying to solve.

In the code above, there’s a number of red flags that have gone up for me, and I tend to look in this area of code first. If these patterns have been used, it tells me the author hasn’t quite grasped how jQuery works and hasn’t considered making simple tuning changes.

The inline return

$.fn.myplugin = function () {
  var me = $(this).each(fn);
  return me;
};

Should be written as:

$.fn.myplugin = function () {
  return $(this).each(fn);
};

The me variable isn’t being used again, so there’s no point in creating it.

Double jQuery

$.fn.myplugin = function () {
  return $(this).each(fn);
};

Whilst within the context of the plugin code – i.e. within the function attached to .fn, the keyword this refers to the jQuery instance, not DOM elements.

If I were to rewrite this to show you the value, you’d see:

$.fn.myplugin = function () {
  return $($('div.foo')).each(fn);
};

So within the actual plugin (not jQuery callbacks), this refers to jQuery, so we can access jQuery’s methods directly:

$.fn.myplugin = function () {
  return this.each(fn);
};

Returning what to each?

$.fn.myplugin = function () {
  return this.each(function () {
    return $(this).bind('someEvent', fn);
  });
};

jQuery’s each iterator simply loops, it doesn’t collect anything. The result variable is jQuery with the original collection inside it still – you can’t modify the collection by returning or not returning.

So return isn’t required at all in this case:

$.fn.myplugin = function () {
  return this.each(function () {
    $(this).bind('someEvent', fn);
  });
};

Wasteful use of each

$.fn.myplugin = function () {
  return this.each(function () {
    $(this).bind('someEvent', fn);
  });
};

Hopefully by removing all the cruft from the starting version, this next step should be obvious. If not, here’s a clue:

  • What’s returned from an each call? A jQuery collection.
  • What’s returned from a bind call? A jQuery collection.

Since we’re running the bind on each element, and only doing that, it means there’s no difference. So let’s throw away the each call and just return the bind:

$.fn.myplugin = function () {
  return this.bind('someEvent', fn);
};

Remember that within the plugin, this refers to the jQuery instance, and not the element, so we don’t need the wrapping $().

All better now, eh?

28 Responses to “Signs of a poorly written jQuery plugin”

  1. Its the little things that you notice about plugins that make the biggest difference… I find I use a plugin as a starting block, rework as suggested above then add my own functionality to it…

    Great read!

  2. It would also ring alarm bells for me that the code is not wrapped in a

    (function($){};)(jQuery);

    This is so easy to do and at least shows someone has thought about compatibility or even just done a bit of googling about plugin design.

  3. All good tips.

    The re-wrapping of the jQuery instance is not only bad because it’s redundant/wasteful but also because it gets rid of the various states preserved in the original object, such as prevObject (accessed via the end method) and selector.

  4. Good stuff man. I think too often new plugin developers read over http://docs.jquery.com/Plugins/Authoring and use its examples as a rigid starting point.

  5. Using namespaces when binding events is also nice and polite.

  6. Yep, some nice examples there Remy. I’d also mention the use of “if” statements where a jQuery each() would do the trick: (particularly where the author does not realise what is being tested by the is() method).

    For example: if( this.is(‘:radio’) ){ do something with this }
    Becomes: this.filter(‘:radio’).each(function(){ do something });

    George

  7. I’m a bit confused by one of your statements.

    Whilst within the context of the plugin code – i.e. within the function attached to .fn, the keyword this refers to the jQuery instance, not DOM elements.

    That seems a bit misleading? Doesn’t ‘this’ refer to the jQuery object created by the selector the plugin is being run on?

  8. @Jeff – that’s what I’ve said isn’t it? this refers to the jQuery object – the current instance of jQuery for the current collection – but in particular, not DOM nodes.

  9. well done sir. I find that your articles are always something I need to hear.

  10. Thanks once more for crystal clear explanation.

  11. great stuff remy, you can be sure i’ll be linking the crap out of this from IRC. :)

  12. Very good tips, I certainly learned a few things.

    Unless you’re a jQuery expert you don’t know a lot of these things, so you’ll just copy code from a tutorial or other plugin. It would be great to create a jQuery plugin tutorial that includes what you have here along with other pro tips, so people have a clean template to start from.

  13. Interesting read, and I must say I’d appreciate your comments regarding some jQuery code I just put out – a slide player to do presentations in html. More details can be found on my blog http://seld.be/notes/introducing-slippy-html-presentations

    It’s not exactly a plugin, but it’s built as a jQuery module of sorts. Anyway any feedback would be welcome.

  14. Great stuff, Remy!

    For those interested, there was a great article on jQuery plugin design patterns on FuelYourCoding a little while back:

    http://fuelyourcoding.com/jquery-plugin-design-patterns-part-i/

  15. Thank you very much. Great points, and made me realize that I perhaps hadn’t spent enough time scrutinizing some of my own jQuery plugins.

    I incorporated some of your thoughts into a new release of my EasyTabs plugin (namely the needless variable declarations).

  16. I don’t think you’re saying that using jQuery#each is always unnecessary when binding events in a plugin. But it should be pointed out that a major jQuery plugin antipattern is lack of support for multiple instances on a single page. If a plugin transforms each affected element by adding element-specific content to the DOM, you *must* use jQuery#each.

    Here’s an example: http://gist.github.com/399181#file_jquery.password_complexity.js

    Thanks for the post. I just tweaked the above example in response to your observation about $(this) within the plugin body. :-)

  17. @Nils – well…actually, in the example you posted I was able to simply refactor the code a little so that your plugin used return this.after(html).blur(fn); (I also saw you had a double document.ready – so I removed that too): http://gist.github.com/424434

    I should add that even though I’ve been able to refactor your code, there’s also a counter argument to use the cached copy of the suggestionElement, rather than running next on each blur – but for this example, I would expect it’s a very low use case (i.e. just for registration) therefore I’d argue that the next can be discounted.

    There are definitely time that you can’t just do return this.fn and you do need this.each – but a lot of the time, actually, you don’t ;-)

  18. I don’t quite agree that having the return statement inline with the rest of the plugin code is necessary. Obviously it works well for the final 1 line of code that you arrive at, but generally it is more logical and clear to separate what you are processing from what you are returning.

    For the initial plugin presented, it is a lot easier to read with return at the bottom as that is where we expect it to be.

    If you are to point out such a change, it would make more sense at the very end of the article, as a last step to producing the final plugin

    Some good tips, otherwise!

  19. [...] Signs of a poorly written jQuery Plugin [...]

  20. “The me variable isn’t being used again, so there’s no point in creating it.”

    actually often i prefer this because it’s easier to view the value while debugging

  21. @Remy: Good points. I agree that there are arguments on both sides with regard to the use of jQuery#next vs using a distinct closure binding for each element’s event sink.

    The double jQuery#ready must have sneaked in there when I was “refactoring” something — oops!

  22. @Remy: Hello Remy. This is really a great post on jQuery Plugin writing. I have translated it into Chinese and I hope more developers in China can benefit from it. You can check it out from: http://deanyan.com/signs-of-a-poorly-written-jquery-plugin/. Besides, I have translated other posts of yours as well. No offensive, all the brainchild is yours and the translated posts having a original link pointing to your site under CCA license.

    Thanks. :)

  23. I have seen many plugin developers write their code ‘wrong’ way (as you mentioned in your examples).

    Great post for any jQuery plugin developer. Thanks Remy.

  24. How would you change what this dude has done over here:

    http://stefangabos.ro/jquery/jquery-plugin-boilerplate-revisited/

  25. Thanks for the insights. Although I knew all this theoretically, I didn’t spot the flaws, when I read the first code example. Theoretical and practical knowledge sometimes differ widely…

    If a jQuery plugin author gets criticized and asks, what’s wrong with his code, this post will become a very good resource to point at.

  26. jQuery’s each iterator simply loops, it doesn’t collect anything.

    That’s not entirely true. You can break the jQuery.each() loop at a particular iteration by making the callback function return false.

    You’re absolutely right that the return is redundant in the case of your example though.

  27. Clashes between jQuery plugins are becoming a serious issue.
    Quality plugin is written by author who understands this fact.
    jQuery plugins introduce all new function names into the single name space.
    This issue is going to bite back jQuery community seriously.

  28. Nice one i have two pluggins for my site a lightbox and an accordion pluggin. it is the same standard as your explanation. and i want to say i have learnt something here.

Leave a Reply
Not required

CODE: Please escape code and wrap in <pre><code>, doing so will automatically syntax highlight