Clean up drupal.js

On an initial read through the changes look good. Nice work. I'll need to apply them, test them, and see if there is any more we can do. Adding this issue to my todo list.

I reviewed and made some updates. Some things I imagine we'll want to discuss.

• Moved all vars to a single var statement at the top of the function.
• Rewrote checkPlain to remove the loop. This provides for significant performance improvements. See http://jsperf.com/check-plain
• Updated our wrapper closure to do dependency injection rather than globals. This should help the file be smaller when minified along with some other good benefits.
• Global functions are really methods on the global object. Rather than using foo() updated to use window.foo(). This is the same pattern that Dojo, jQuery, etc use.

Thoughts?

Hi,

this is really exciting to see efforts in pushing Drupal js into a more modern and robust way, and I would definitely like to give time for it.

I applied the patch in #5, and after some testing, no error / misbehavior so far.
Both patch in #1 and #5 looks really good improvements, compliments.

- Patch in #1
in Drupal.behaviors the function type test is gone, is it intended ?

$.isFunction(this.attach)
$.isFunction(this.detach)

A couple of quick ideas for the next steps :

- Should we advocate on the use of strict comparison operators everywhere ? There's some discussion around it in the js community.
- As there might be more and more functions / utilities added the the Drupal object, leading toward a sort of small framework, should we compare the different module patterns / inheritance APIs ?

@opalescent You bring up some good points so let me try to address them.

First, the removal of the use of $.isFunction. While I cannot speak for ksenzee, I am not sure I see a reason for isFunction here. There is nothing in the API that would put anything but a function on Drupal.behaviors.example.attach/detach. If someone did so it would be considered a bug in that code so throwing a JS error is OK by me. And, since we are making sure only properties on the behaviors are being looked at (via hasOwnProperty()) outside code should not slip in to make sure we avoid.

Second, the comparison operators. There is not complete agreement on the best approach. I did a quick analysis of jQuery, YUI, and dojo on this. Seems jQuery uses ===/!== for everything but comparisons with null where == is used. Dojo uses == regularly for comparisons. YUI intermixes the use of == and ===. I'm not yet entirely sure of what we should do other than to suggest we default to === and fall back to == when it makes sense to.

Third, the patterns. This issue and the set we are working on right now are to clean up the existing code. In the JS community initiative we've separated cleanup from changing the architecture for some better patterns. So, yes we need some better patterns. This is just not the issue to hash it out. We don't want to put too many changes in one patch/issue and the patterns one will require a bit of research, discussion, and finding agreement. So, it's separated out.

Another possible change we can make is to move _enhancejQuerySupport() off of the Drupal object into a private var in the closure. It used strictly for setup code, which should not be bassed a second time. Thoughts?

Thanks for clearing up all those points. I have to say I completely agree with you on them.

The removal of the use of $.isFunction, totally make sense to me too, we shouldn't write code that think in place of the end user, but rather transfer some responsibilities to him, and bring the tools, documentation to achieve it.

Comparison operators, I'll try to bring good infos sources about it, so we can consider the arguments for or against. But I like your suggestion. Use the strict as a basis, but stay flexible to use cases.

The patterns. Sorry, I did not wanted to rush things. I had read your good blog posts, and also the initiative page, but it did not came in mind last time, was too late maybe...
I can only say, thank you for writing such a clear and smart working plan.

Regarding _enhancejQuerySupport(): once again, make sense, no need to have it populate the Drupal object while it's called only once.

@opalescent I was thinking the exact same thing about _enhancejQuerySupport()

This patch cleans up some ==/===, moves enhancejQuerySupport() off the Drupal object (I imagine this might cause some discussion), and uses a DOM element to figure out positionFixed (which is faster than using a jQuery object). This is in addition to the previous changes.

Anyone up for testing/reviewing?

Patch in #11 applied and tested (manually) and everything's fine in the browsers I tested in (FF, Chrome, Safari, Opera). I'll do some IE later on.

I'm particularly keen on the changes executed, so no critics from me.

Rewriting the enhancejQuerySupport() with native DOM elements/functions is what I've been planning to as well, so you was faster.
On question, although it seem to work nicely, the element is not injected inside the body element, which is not ready at the function execution time, but rather in between html and head. Just wondering whether this could be a concern or not. Does not seem so.
http://kangax.github.com/cft/#IS_POSITION_FIXED_SUPPORTED
https://gist.github.com/362170#L32

Looking at what paul created, we should hide our element and account for page scroll. I'm wondering if enhancejQuerySupport() is now way more difficult to debug and understand. For the performance improvement I wonder if its worth it?

Tagging...

+++ misc/drupal.jsundefined
@@ -43,18 +43,20 @@ jQuery.noConflict();
+    if (behaviors.hasOwnProperty(i) && behaviors[i].attach) {
+      behaviors[i].attach(context, settings);

There might be a lot of behaviors in a given drupal site. Does it make sense to cache the hasOwnProperty lookup.

Something like this:

Drupal.attachBehaviors = function (context, settings, trigger) {
  var behaviors = Drupal.behaviors,
        hasOwnProp = Object.prototype.hasOwnProperty,
        i;
    ....
   for (i in behaviors) {
    if (hasOwnProp.call(behaviors, i) && behaviors[i].attach) {
      .....
     }
  }
 };

This isn't an actual review I was just looking at the patch and that caught my attention.

#16 not really, generally using .call hurts a more than a method lookup : http://jsperf.com/object-hasownproperty-vs-hasownproperty/2 needs some IE testing though.

I'm not sure about the function check: $.isFunction has to go I agree, changing that to typeof could be a good middle ground, it's a pretty critical part, it doesn't hurt to protect users against themselves. It's always possible to throw an exception or something too.

The $.support thing should just go away, it's buggy #1439462: Drop our custom $.support.positionFixed and use jQuery's $.support.fixedPosition.

The patch in #11 is extremely stale, so I rerolled. A good chunk of it was already implemented with drupal.js’s new Drupal.formatString().

#13: The changes to enhancejQuerySupport seem a little beyond the scope of cleanup. It seems a little dicey to me that we're suddenly running the function as soon as the script loads, instead of waiting for the DOM to be ready. I'm wondering if we could leave that for a separate issue with its own discussion.

Unless I'm misreading this code:

$(function () {
  /**
   * Boolean indicating whether or not position:fixed is supported.
   */
  if (jQuery.support.positionFixed === undefined) {
    var el = $('<div style="position:fixed; top:10px" />').appendTo(document.body);
    jQuery.support.positionFixed = el[0].offsetTop === 10;
    el.remove();
  }
});

We are already running that function as soon as possible. Note that it isn't wrapped in $(document).ready(). mfer's cleanup just puts the enhancejQuerySupport name around it and cleans it up.

#5-3: I'm not a huge fan of the file-level closure pattern anyway. It's bad DX and it's never commented, so nobody really understands what it's doing unless they get functional programming to a certain degree. So they just copy and paste and grumble. But I won't argue about it, since theoretically someday we'll be minimizing our code and it'll help there. Maybe we could come up with a way of documenting it clearly and add a comment with a link?

I never really understood the (function ($) { // stuff })(jQuery) stuff until I read JavaScript Module Pattern: In-Depth. If some bit of code is the right thing to do, but hard to understand, then we need some code commenting. I think http://drupal.org/node/224333#javascript_compatibility could use some improvement, for example.

So, let's add some code comments.

Here's the rerolled patch in #11 without code comments.

I just updated http://drupal.org/update/modules/6/7#javascript_compatibility to read:

JavaScript should be compatible with libraries other than jQuery

(issue). JavaScript should be made compatible with libraries other than jQuery by adding a small wrapper around your existing code:

(function ($, Drupal, window, document, undefined) {
  // Original JavaScript code.
})(jQuery, Drupal, this, this.document);

This wrapper is an anonymous closure that provides state throughout the page's lifetime and ensures your code does not unintentionally create/override global variables.

It also explicitly imports the global jQuery variable so that your code can use the local $ variable instead of the jQuery global. This is essential because Drupal loads jQuery with noConflict() compatibility so the jQuery library does not setup the normal $ as a global variable.

The wrapper above also explicitly loads the Drupal, this, and this.document globals so that they can be referenced as Drupal, window, and document, respectively. It is good JavaScript coding practice to explicitly list the globals your code is relying on in this way.

A detailed explanation of this JavaScript pattern can be found in the article JavaScript Module Pattern: In-Depth.

[Edit: I reverted a part of that D6->D7 update since its not actually how D7's code looks. :-( But I moved that exact language to a D8 change record. http://drupal.org/node/1446420 ]

I was going to add a link to the 6/7 upgrade, but that seems odd for D8 change. So I added a change record. http://drupal.org/node/1446420

Here's the patch with the code comment.

Thanks for the reroll and all the documentation :D

Couple of things now that it's easy to look at the diff :)
There are changes that should be made globally, not just in drupal.js: #1428534: Use === and !==, #1428524: Replace all $.each() with filtered for loop and the way vars are declared. There are issues for the first two, for the last I guess we have to decide if we what that everywhere or not and if we use only one var on top or allow several var statements.

Ideally I'd go with one var but that's a very easy way to have leaking vars, it's easy to put a ";" instead of "," during the declaration. Since it's dangerous I'd say we go with 1 var per line and multiple variable declaration per line allowed.

The position fixed support addition is dead to me, i'd be happy if that was removed #1439462: Drop our custom $.support.positionFixed and use jQuery's $.support.fixedPosition.

For me this patch should be reduced to doc changes, function check in attachBehaviors, closure definition and check plain perf optimization. Good news is that we all agree on what should be changed :D

There is now an issue for var declarations #1483396: [policy, no patch] JavaScript coding standards for variable declarations.

reroll, I left out the === change because it's in another issue linked above.

+++ b/core/misc/drupal.js
@@ -3,7 +3,9 @@ var Drupal = Drupal || { 'settings': {}, 'behaviors': {}, 'locale': {} };
+// JavaScript should be made compatible with libraries other than jQuery by
+// wrapping it in an anonymous closure. See http://drupal.org/node/1446420

Let's remove the See and URL. This does not really need further explanation, and people who want to find out why it exists can grep/pickaxe the commit history.

I'm fine with the remainder of the comment, as long as it is added in drupal.js only.

+++ b/core/misc/drupal.js
@@ -14,25 +16,26 @@ jQuery.noConflict();
- *        ...
+ *        …

@@ -263,7 +270,7 @@ Drupal.formatPlural = function (count, singular, plural, args, options) {
- * @param ...
+ * @param …

Can we revert these, please? We don't use typographic ellipses anywhere else in Drupal's source code base (except of human-readable strings being output).

thanks, reroll

woops, forgot the See+url

Awesome, thank you!

I wanted to mention we need a change notice for this, but the existing https://drupal.org/node/1446420 has been changed and referenced to this issue. I'm not sure whether that's wise. There's tons of JS code written for D7 already, and while the change notification is no functional change, I can see quite some confusion popping up in contrib (à la bug reports along the lines of "Improper JavaScript closure")...

This looks great. I'm going to stick it in the change notification queue just in case, couldn't 100% parse #28.

Committed/pushed to 8.x.

don't really get #28 either, change notif seems clear enough to me, can we close this?