JavaScript Patch #2: Weight

subscribe

The weight matters in javascript. A js file can define a function and then a later file can override that function. For example, a module could provide the function:

myfunction = function(args) {
  // do something
}

Later in the execution order a theme js file could override that with something like:

myfunction = function(args) {
  // do something different
}

So, there are cases where the execution order matters. It's just not a typical situation and may not be that commonly used.

#317122

Here's what I noticed from a quick review:

• There seems to be an inconsistency when adding js files that are in the misc directory. Sometimes they are added as 'core' files and other times they are added as the default type (module). This happens before and after the patch. Should we make them consistent as being 'core'? This would stop them from being included more than once on accident. I didn't make any changes for this. Thoughts?
• Noticed the tests are designed for when preprocessing is turned off. They will fail (before and after the patch) when preprocessing is on. Fixed this. Also, made sure to call parent::setUp() in the JS test method setUp().
• Fixed the tests.
• We should add in some tests for preprocessing of js. That may be another issue.

I hesitate to set this RTBC because of my first bullet in #8. If we leave in the inconsistent weight of the files in the misc directory it looks good to go. If we move all the files in the misc directory to the core weight some JavaScript breaks and we will need to clean that up. I'd like to clean up the js files. If we go down that route the attached patch sets the core weight on all the files in the misc directory.

Enable the locale module and run the JS tests. You should see them throw exceptions just before drupal_get_js() is called. drupal_get_js() calls a locale function if it's available and that still needs to be updated.

This patch updates for a moving head and updates locale_update_js_files() to the new $javascript array structure. There is still an exception poping up when the tests are run with the locale module enabled.

Fixed error in testing environment. locale needed to be enabled via parent::setUp('locale'); in the JavaScript tests.

+ *       Defines the order in which the JavaScript is added to the page. Defaults
+ *       to JS_WEIGHT_DEFAULT.

- Extend the PHPdoc above to mention why defining the order is useful/important.
- List the other available weights, and explain why they are added in the current order.

+ drupal_add_js('misc/tabledrag.js', array('weight' => JS_WEIGHT_DEFAULT - 1));
- Add a code comment to explain the '- 1'.

Otherwise looks good.

I noticed this but that's how I wanted the behavior. We have files, then settings, and then inline JavaScript. Within the files and inline types it is weighted. I think this is how it should be. Otherwise, when we get into JavaScript preprocessing it will get complicated.

Maybe we could/should have a comment about this behavior in the code.

I have posted a patch focusing primarily on the internal implementation of these functions as well as trying to make the CSS management code and API more consistent over at http://drupal.org/node/251578#comment-1083396

I think we still need to break it into smaller patches of course, but I feel like we need a more coherent vision of what we are aiming at (and how we bring CSS along for the ride), so I hope that looking at that code as a point to aim at and perhaps it would be worth incorporating some of the concepts from that patch into this one would be beneficial.

The patch works, passes tests, and seems in order.

An issue brought up in IRC is with an array keying the way this patch does it we will have a hard time with #315801: JavaScript Patch #3: JS Alter Operation.

Doh, I forgot... it doesn't apply cleanly anymore. Head moved.

Looks good to me. Tests pass. Nice work Rob.

mfer and Rob, what do you think about #251578-90: More flexible js/css ordering and an alter operation?

@dries - I had a chance to talk with Grugnog2 in irc shortly after he posted the patch and had a change to look over the patch. It doesn't apply against head so I didn't apply and take it for a spin yet. Managing JS and CSS happens in a very similar way in drupal. His patch looks to move them to a common workflow which I really like and it makes implementing #315801: JavaScript Patch #3: JS Alter Operation simpler. This patch, like the previous ones in that thread, is a mega patch that deals with weights, alters, and re-factoring the API.

I would suggest committing this patch which sets the weight levels and a method for weighting. Then we move on to re-factor the API for JS and CSS following #251578-90: More flexible js/css ordering and an alter operation and then we pick up with the other JS imporvement patches.

This should make for less maintenance and work. Currently, each time we make a change to drupal_add_js, like when we moved it to use a $options array we then go make the same change to drupal_add_css. Thoughts? Rob?

@Rob Loach I was looking at how the patch in #251578-90: More flexible js/css ordering and an alter operation sorts by weight and I think it might be easier for the alter functionality and more consistent with the way the rest of drupal does weights. Thoughts?

Also, you might be on to something with moving the JS system to where we want it to be and then making it common enough to use with CSS if that works. That might be an approach that gets us there with less issues, if it works to go that route at all.

HI Folks...

I have posted an updated patch in #251578-92: More flexible js/css ordering and an alter operation - if you could take a look that would be great. I am beginning to feel like it not be crazy to commit that patch, if we can get some momentum behind it to polish off the DX and docs. As we discussed on IRC I think that while this one moves us along a little, it is very JS specific (leaves CSS behind), and doesn't really provide a useful way to add an alter hook. I do think there are a couple of features from this one, like the constants for the specific values, some of the documentation and also the simpletests (already merged into the other patch) that we can keep.

Setting back to RTBC. Can we get this committed so we can move on to the next step in the JS changes. Smaller patches would be much better than a large one and this will get the weights in, even if we modify them slightly later.

@Grugnog2 - can you, please, break out the API change in your patch into a separate issue that doesn't try to take on everything at once? One that just handles the API change and not everything in one patch.

Discussed on IRC - it is simpler to combine the API changes into this patch, since they are essentially the exact same lines changed with 2 subtle semantic differences to make it easier to apply with when we rework the internals.

Here are the changes from the #26 patch:
* Option elements are now prefixed with '#' - this allows us to use the standard ucsort helper function to sort (rather than sorting using weight with array keys as done in this patch, which as noted on IRC will make it hard to have a useful alter function).
* 'type' is now called 'method' - this is because we need 'type' later on to delineate JS vs. CSS - no callers were actually passing this in as part of the array (just as a string second parameter) so this change is mostly just documentation.
* I also found a drupal_add_js() in install.php that had been missed.

All tests pass.

After yet more discussion on IRC, we are leaving #method as #type for now. (Edit: missed a couple)

I'm curious to know what some other JS folks think about this approach.

Back in Drupal 5/Drupal 6, I went on a crusade to add #properties everywhere, because as you know I am the Consistency Queen. :P But I got slapped down by Steven (and I think others too) because he pointed out that we should only ever use #properties when we /have/ to use them, and use regular arrays everywhere else. This is mainly because #thingy is a Drupalism and slaps a barrier in front of people with existing PHP knowledge. I also dislike it here because it's inconsistent with the way functions like l() and url() treat their $options arrays, so it's yet another quirk developers would need to know.

We /have/ to use #properties in FAPI because you can arbitrarily nest elements, and you need a way to differentiate the elements in the array from their properties. In other words, you need to be able to do both of these:

$form['foo_field'] = array(
  '#type' => 'foo',
  '#title' => t('Foo'),
  ...
);

$form['fieldset'] = array(
    array('field_foo'] = array(
      '#type' => 'foo',
      '#title' => t('Foo'),
      ...
    ),
  );

Without #properties, it would be impossible for FAPI to know when to stop treating array indexes as form elements.

Er. That was an exceptionally long way of saying, "I'd prefer to just use 'weight' => BLAH" if we can. :) Rob pointed out that using #weight allows re-use of the element_sort() function, as well as easy passage into drupal_alter(). However, element_sort() is thus called because it's intended to sort form elements, and JS options are /not/ those. We can make another function like drupal_array_set_weights() or something less verbose for these kind of general things.

Or look at how menus are done, since they don't use #properties and are both weighted and alterable.

@webchick - While I have not had a chance to work through this patch yet, we (@Rob Loach, @Grugnog2, and myself) talked about this in IRC. This way of organizing and sorting the $javascript array will lead to much easier to implement alters and if/when we rework the internal of the JS and CSS functions to use the same helper functions. Plus, the array structure is more consistent with the rest of core and easier to grok.

Just imagine (yes, I am getting ahead of myself here) something like:

function jquery_update_js_alter(&$js) {
  $js['/path/to/new/jquery.js'] = array(
    'data' => '/path/to/new/jquery.js',
    'type' => 'file',
    'scope' => 'header',
    'weight' => JS_WEIGHT_LIBRARY - 1,
    'cache' => TRUE,
    'defer' => FALSE,
    'preprocess' => TRUE
  );
  unset($js['misc/jquery.js']);
}

As mentioned in IRC, I found Grugnog's approach appealing. He used #properties only to be able to use element_sort(). However, drupal_get_js() and drupal_get_css() produce output - and any output in Drupal goes through theme functions normally. Hence, by using #properties, we could leverage all the benefits and flexibility of drupal_render(). A) Weights. B) prefix/suffix for inline scripts and settings. C) Pluggable theme system. D) hook_elements() to inject a #pre_render callback, f.e. allowing CDN modules to alter the data before it is rendered.

The only valid caveat I can see is webchick's about being inconsistent with options for l() and url(). However, one could even argue that l() is producing output, too...

The JS_WEIGHT_LIBRARY & co. constants are a bit too lengthy IMO. Can't we drop "WEIGHT_" in there?

Could we limit further patches to the API changes only, so we can discuss the approach first? It seems we're wasting much time here in altering all invocations of drupal_add_js() and tests throughout core for each patch, while we are still debating about the way to go. Also, tracking the important changes in a 30-50 KB patch is very hard. :(

Lastly, could you please use diff -up to create patches? It's hard to review them without knowing which function is altered.

@sun - I'd like to not go too overboard with the drupal js functions. Let's try to keep them as simple as we can while filling in all the features we need while keeping DX high. Having a # at the start array keys is lower DX for most php developers.

I agree on dropping the WEIGHT from JS_WEIGHT_LIBRARY and the others. I think it's descriptive enough that we have 'weight' => JS_LIBRARY. The Extra WEIGHT may be a bit redundant and I like less characters to type.

In order to add a CDN we don't need a #pre_render. Just do a drupal_alter on the $javascript array as we are planning to in #315801: JavaScript Patch #3: JS Alter Operation. Something like:

function cdn_js_alter(&$js) {
  foreach ($js as $key => $value) {
    if ($value['type'] == 'file') {
      $js[$key]['data'] = CDN_PATH . $js[$key]['data'];
    }
  }
}

The real question is, do we need a tree structure for the $javascript array? Is there a use case for that? Every use case I came up with was overkill and not really needed. If not, using drupal_render is a bit of an overkill.

I totally agree with the discussion so far. As it stands (with the new sort helper) I am pretty sure the patch implements all the API changes necessary for refactoring of the internals as I proposed in #251578-92.

The one change I would suggest is that jquery gets a weight of JS_LIBRARY - 20, and drupal.js gets a weight of JS_LIBRARY - 10. It seems likely that we will have sub-libraries that build off some of the functions (e.g. string translation) in drupal.js, so it would be worthwhile ensuring that drupal.js comes first. Also, adding a little more space never hurts - we aren't exactly close to any integer bounds yet ;)

Since jquery.js and drupal.js are added first (right at the beginning of the first drupal_add_js call) and have a weight JS_LIBRARY is there a case where they wouldn't end up being the first 2 items in the rendered JS as it is?

@Rob Loach - I could go either way with JS_ENGINE, but I suggest we give drupal.js an explicit weight (literal or a constant) putting it in between jquery (that it relies on) and the other libraries (that might rely on it's functions) - leaving it with JS_LIBRARY could place it after other libraries.

@mfer - from the usort() function documentation http://php.net/manual/en/function.usort.php "Note: If two members compare as equal, their order in the sorted array is undefined." - we are using uasort(), but I am pretty sure the same rule would apply. In other words, we are relying on a side effect of the current implementation which could change in new versions of PHP - better to be explicit I think.

drupal.js should come before the other libraries in case they want to use its functions. uasort seems to sort based on the weight and then the order in which the array was altered. If we add jquery.js with a weight of library - 1, the drupal.js with a weight of library, and then something like farbtastic.js with a weight of library, and then alter the array item for drupal.js it will put it after farbtastic.js in the sorted array. This may not be an issue now, but as soon as we move to the alter patch it will have an effect.

As Grugnog2 points out, this behavior is in my version of PHP but may not be the same in every version.

The attached patch moves jquery.js to JS_LIBRARY -2 and drupal.js to JS_LIBRARY -1. These are the only differences from #43. We can go with this one. Or, if this is scope creep from the alter patch we can move it to there and go with the patch in #43. It does pass despite the automated message saying it doesn't.

I tested #43. The tests pass, everything seems to work, and the approach is pretty slick (nice job Rob Loach).

Updating patch to HEAD - 1 chunk was not applying.

This patch is breaking tests, which I am assuming the result of some other patch that just got in - the cause appears to be parent::setUp('locale'); in the test setUp, which is somehow causing the simpletest table to not be found.

Tests pass, tabledrag, fieldsets and color module all work fine.

Sorry.

This looks like the rest of the "Add a file" description is outdated:

- * - Add a file ('core', 'module' and 'theme'):
+ * - Add a file ('file'):
  *   Adds a reference to a JavaScript file to the page. JavaScript files
  *   are placed in a certain order, from 'core' first, to 'module' and finally
  *   'theme' so that files, that are added later, can override previously added

Regarding:

+ *   - weight
+ *       A number defining the order in which the JavaScript is added to the
+ *       page. In some cases, the order in which the JavaScript is presented
+ *       on the page is very important. Behaviors, for example, require
+ *       registration before being put to use. Defaults to JS_DEFAULT.
+ *
+ *       Possible constants are as follows...
+ *       - JS_LIBRARY: Any libraries, settings, or jQuery plugins.
+ *       - JS_DEFAULT: Any module-layer JavaScript.
+ *       - JS_THEME: Any theme-layer JavaScript.
+ *       If you need to invoke a JavaScript file before any other module's
+ *       JavaScript, for example, you would use JS_DEFAULT - 1. Note
+ *       that inline JavaScript will be outputed after files regardless of
+ *       their weight.

The behavior example is a bit fuzzy. "JavaScripts registering behaviors in Drupal.behaviors..." - erm - I cannot imagine a special case for behaviors, besides being loaded in JS_DEFAULT (i.e. after JS_LIBRARY) so we might drop that example.
Please change to "Available constants are:", and add a blank line after the list.
Also, "outputed" should be "output". Aside from that, I did not grasp this note about inline JavaScripts after reading it. Where is inline JS ouput?

Missing trailing comma (not only in this example):

+        'settings' => array(
...
+          'weight' => JS_LIBRARY

First line of PHPdoc blocks needs to be on one line, regardless of its length:

 /**
+ * Function used by uasort to sort structured arrays by weight, without
+ * the property weight prefix.
+ */

A quick search in the API revealed that we already have two similar sorting functions in core. Can we rename this function to sort_weight() or drupal_sort_weight() to make it re-usable? ("element" is inappropriate)
http://api.drupal.org/api/function/_user_sort/7
http://api.drupal.org/api/function/_system_sort_requirements/7

+function element_sort_weight($a, $b) {
+  $a_weight = (is_array($a) && isset($a['weight'])) ? $a['weight'] : 0;
+  $b_weight = (is_array($b) && isset($b['weight'])) ? $b['weight'] : 0;
+  if ($a_weight == $b_weight) {
+    return 0;
+  }
+  return ($a_weight < $b_weight) ? -1 : 1;
+}

Please note that "scope" should be renamed to "region", which will be treated in #330985: Rename "scope" to "region" in drupal_add_js()

Just a note that I got derailed tonight by the test failures caused @ #243532: Catch notices, warnings, errors and fatal errors from the tested side, and have to make fixing those my top priority. However! Once those are in, this is my very second priority. :) Thanks for your patience, and keep up the awesome reviews!

In the examples for drupal_add_js there is no example for adding a setting. But, this isn't really a weight patch issue.

This looks good to me. The only question I have is for a maintainer, is drupal_sort_weight the appropriate function name here? If so, this patch should be good to go.

Just to add my two cents worth, I've tested this patch and had a go at adding the jquery ui library and some inline js, altering weight (using constants) and scope, etc. All seems to work very well - it's great to have such control over where your scripts appear.
One comment I would have is on some slightly confusing documentation - is it just me or is there something strange about saying that the 'type' key can have the value 'setting' but that this key is not valid when the type is setting. Or am I reading this wrong..

+ *   in the $data parameter ('file'/'setting'/'inline'), or an array which
+ *   can have any or all of the following keys (these are not valid when the
+ *   type is 'setting'):
  *   - type
- *       The type of JavaScript that should be added to the page. Allowed
- *       values are 'core', 'module', 'theme', 'inline' and 'setting'. Defaults
- *       to 'module'.
+ *       The type of JavaScript that is to be added to the page.
+ *       Allowed values are 'file', 'inline' and 'setting'. Defaults to 'file'.

Agreed that comment could use a little work. I asked katbailey to take a look at this patch since she's a JS whiz who hasn't been embroiled in this since the beginning. :) Great to know it holds up well under fire!

I have looked through the rest with a fine-toothed comb at least 2-3 times now, and think we've discussed all of the various areas. I am therefore pretty confident that we can press the big green button the next time this rolls over to RTBC! :D

Although technically incorrect, I've changed and simplified that to:

 * @param $options
 *   (optional) A string defining the type of JavaScript that is being added
 *   in the $data parameter ('file'/'setting'/'inline'), or an array which
 *   can have any or all of the following keys if the type is 'file' or
 *   'inline':
 *   - type
 *       The type of JavaScript that is to be added to the page.
 *       Allowed values are 'file' and 'inline'. Defaults to 'file'.

"Although technically incorrect ..."

What does that mean? :)

I'm sure we can do clear and technically correct ;-) How about:

 * @param $options
 *   (optional) A string defining the type of JavaScript that is being added
 *   in the $data parameter ('file'/'setting'/'inline'), or an array which
 *   can have any or all of the following keys (though scope and weight are ignored when the
 *   type is 'setting'):
 *   - type
 *       The type of JavaScript that is to be added to the page.
 *       Allowed values are 'file', 'inline' and 'setting'. Defaults to 'file'.

IMHO we should shortcut the information flow. If I want to add settings, all I need to know is how, and that it does not make sense to pass an array for $options. Technically, I could do array('type' => 'setting'), but no other option is taken into account. Therefore, reverted the notes for options that do not apply to settings and changed to:

 * @param $options
 *   (optional) A string defining the type of JavaScript that is being added
 *   in the $data parameter ('file'/'setting'/'inline'), or an array which
 *   can have any or all of the following keys. JavaScript settings should
 *   always pass the string 'setting' only.
 *   - type
 *       The type of JavaScript that is to be added to the page. Allowed
 *       values are 'file', 'inline' or 'setting'. Defaults to 'file'.

With "should" we are communicating the best practice. If, for any reason, a developer tries to pass other options for settings and wonders how those affect the settings, she can look up the trivial code for settings and learn that none of the other options would make sense, since all settings are just merged into one big array.

Please note that I've additionally fixed some minor coding-style issues (missing blank lines between switch cases).

Committteeeeeeedddd!! :D

Awesome job, folks!!! Thanks so much for powering through this 'til the end. :)

Oh wait.

Upgrade docs please. ;)