All .js in /misc should be registered as a library

I don't know if we need it for *every* JS file in "misc", though I'm not opposed to doing it for all of them either. But definitely +1 for doing it for ajax.js, and any other file that needs to be included with "group" => JS_LIBRARY.

I am not attaching a patch because I want to get some feedback and make sure that this API change can get in before I spend the time doing it.

Even if we did it for every misc js file, I'm not sure how that would constitute an API change. As stated in the OD, already ported modules that call drupal_add_js() directly would continue to work the same way they currently do. But sadly, I don't have the power to make any promises on what can or can't get in.

I fully support doing this. We're not breaking anything, we're just providing a way for contrib authors to include their files easily, with the proper dependencies in place. In fact, I just discovered today that tabledrag.js has a dependency on jquery.cookie.js. If nothing else, we need to create libraries for tabledrag and collapse, and anything else we find, to get the dependencies in place. But it would be better to create the libraries in advance.

BTW, I also think this is good practice for contrib. See, for example, #956398: Simplify webform-admin.(js|css) inclusion by using hook_library(), and fix 'preprocess', 'group', and 'weight'. That, of course, is up to each contrib module maintainer to decide, but maybe we'll have opportunities to encourage this behavior via docs, books, conference sessions, etc.

Having experienced this problem in contrib myself already, I think this is major.

+++ includes/form.inc	12 Nov 2010 17:11:42 -0000
@@ -3533,7 +3533,7 @@ function theme_textfield($variables) {
-    drupal_add_js('misc/autocomplete.js');
+    drupal_add_library('autocomplete');

@@ -3595,7 +3595,7 @@ function theme_textarea($variables) {
-    drupal_add_js('misc/textarea.js');
+    drupal_add_library('textarea');

Missing first 'system' argument.

+++ modules/system/system.module	12 Nov 2010 17:11:45 -0000
@@ -1076,6 +1076,90 @@ function _system_batch_theme() {
+      'misc/batch.js' => array('cache' => FALSE),
...
+      'misc/progress.js' => array('cache' => FALSE),

FWIW, I never understood those 'cache' => FALSE parameters. Separate issue, I know, just mentioning.

+++ modules/system/system.module	12 Nov 2010 17:11:45 -0000
@@ -1076,6 +1076,90 @@ function _system_batch_theme() {
+  // Drupal's form library.
+  // note: Because jQuery got the form namespace first this is oddly named.
+  $libraries['drupal-form'] = array(

Would be good to also convert drupal.js and name that "drupal", which in turn allows us to rename "drupal-form" into "drupal.form" for naming consistency with jQuery (UI) libraries.

Powered by Dreditor.

subscribe

Let me see if I can update this this afternoon.

This fixes the errors sun noted in #7. It also changes the namespace, so we have drupal.ajax, drupal.batch, drupal.form etc.

That also means that we should document that all of the jquery ones are misnamed and that they SHOULD be jquery.foo but aren't because it's too late to actually change them.

It's too late to change them, but it seems acceptable to add the sensible names, to avoid a bit of a DX WTF. So I added jquery.form, jquery.cookie, jquery.once, jquery.bbq, and drupal.vertical-tabs; documented that the original names are deprecated; and updated core to use the new more sensible names.

What I didn't do is make drupal.js into a library. I started to, but then decided it would probably never be used, and putting code in core that we never expect to be executed seems like an unwise move. The drupal.* namespace makes sense without it IMO.

In IRC sun suggested using references in system_library() to save space in the array, and we double-checked that it still serializes and unserializes correctly (thanks dmitrig01). So here's a revised patch with just that change.

subscribe

Looks ready to fly for me. However, didn't test manually.

Manually tested, everything works like it should, so let's get this badly needed patch in.

yes, I second that.

Thought about it quite a bit but ultimately decided to proceed with this. Committed to CVS HEAD.

I committed this to CVS HEAD, but I'm marking it 'needs work' until someone has updated the documentation. Feel free to mark this 'fixed' when it is properly documented.

Adding "Needs Documentation" tag.

Tagging for sprint as an issue that needs to be documented in the 6/7 module update guide

This needs to be announced as well, with some more details. From merlinofchaos:

I think the ajax library patch probably deserves a development list mention, that it should not longer be considered okay to use drupal_add_js on most Drupal .js files now and that instead drupal_add_library or ['#attached']['library'] should be used instead, and point to system_library() for a list of available libraries. This will ensure that library order and dependencies are properly fulfilled. Using drupal_add_js() could potentially alter/damage this order.

I'd be interested in hearing more about strategy in this area with regard to the final changes in #769226: Optimize JS/CSS aggregation for front-end performance and DX.

What am I doing here... Assigning to myself. Maybe for the docs sprint.

Changing tags

+1 subscribe

Please review this update in the 6/7 module upgrade guide.

I also added about loading in .info files in the D7 Managing JS page.

I think (correct me if I'm wrong) that saying drupal_add_js() is "not recommended at all" is perhaps a little strong. Presumably if a module for some reason had some JS that was dynamically generated based on what was in the page, and was added to just some pages, adding it with drupal_add_js() would be the only way to accomplish that. And perhaps there are other reasons?

And if we do want to recommend people not use drupal_add_js() in such strong terms, we should add that to the drupal_add_js() function doc. And probably to the drupal_add_css() doc too?

I'm also a bit concerned with this update guide section, because it doesn't explicitly say what is different between 6/7 in this section, or why the change in how to do things is a good idea, as suggested in #21.

rfay: also have you announced this by email too? Probably should leave at active or needs work until that is done.

@jhodgdon, yeah, I'm going to get merlinofchaos to weigh in on what his intent was re drupal_add_library(). After he and ksenzee think we're saying the right thing I'll announce it. Thanks for your comment!

Thanks, @merlinofchaos. Updated the info -

Please review this update in the 6/7 module upgrade guide.

I also added about loading in .info files in the D7 Managing JS page.

The dependencies were not done (or are not complete). For example, form.js has a dependency on jquery.cookie.js. If the cookie plugin is not present there can be errors.

@mfer: Nice catch. That's exactly the reason for why this change was very important to do.

+++ includes/common.inc
@@ -4636,7 +4636,7 @@ function drupal_add_tabledrag($table_id, $action, $relationship, $group, $subgro
     // Add the table drag JavaScript to the page before the module JavaScript
     // to ensure that table drag behaviors are registered before any module
     // uses it.
-    drupal_add_js('misc/jquery.cookie.js', array('weight' => -2));
+    drupal_add_library('system', 'jquery.cookie');
     drupal_add_js('misc/tabledrag.js', array('weight' => -1));

Looks like we also forgot to register drupal.tabledrag as library?

Powered by Dreditor.

I think the update doc is fine now, and this is back to being a coding issue.

I happened across an instance in user.admin.inc. Shouldn't this be drupal_add_library()?

@@ -99,7 +99,7 @@ function user_filter_form() {
     );
   }
 
-  drupal_add_js('misc/form.js');
+  drupal_add_library('system', 'drupal.form');

Here's a patch with the feedback from #30 and #34.

@sun, I thought about declaring tabledrag as a library. I didn't end up doing it, because the public API for getting tabledrag onto your page is drupal_add_tabledrag(), not drupal_add_library(). But if you think it makes sense as a library we can certainly add it.

I didn't end up doing it, because the public API for getting tabledrag onto your page is drupal_add_tabledrag(), not drupal_add_library().

Good idea - makes perfectly sense.

Committed to HEAD. Thanks!

The originally committed patch in this issue (#12) led to #986298: fieldsets inside of vertical tabs on the form prepend "Show/Hide" to the titles that are displayed in the vertical tabs, which is a pretty subtle bug that apparently has to do with the order JavaScript is loaded on the page.

Because of that, it makes me wonder whether this caused other subtle bugs as well.

The problem is that anything added with drupal_add_library(), by default, gets put in the JS_LIBRARY group, whereas anything added with drupal_add_js(), by default, gets put in the JS_DEFAULT group. So, #12 inadvertently changed the group of some JS files from the later JS_DEFAULT to the earlier JS_LIBRARY. This fixes it.

This adds a test for the issue in #986298: fieldsets inside of vertical tabs on the form prepend "Show/Hide" to the titles that are displayed in the vertical tabs. I'll now mark that one a duplicate.

The patch looks good and fixes the bug. And comparing it to the original patch, it seems to get all the ones that it needed to. (It doesn't 100% result in the same ordering of JavaScript files as before, but it gets the important ones right and we don't know of any other bugs so that's probably fine.)

It's a little odd that we essentially have two classes of libraries now: The "real" libraries (that get weighted like libraries) and then some others which are registered as libraries but aren't quite as generic so they get weighted higher. I can sort of intuitively understand which is which, but that's a little weird. I wonder if there is some criteria for distinguishing these which could be explained in a code comment. That's the only thing preventing me from marking this RTBC just yet - otherwise it's good to go.

When drupal_add_library() was designed and implemented, we purposively left out regular module assets, as those are not libraries. The primary purpose was to be able to load a definite bundle of jQuery UI library assets, including dependencies, from /misc/ui.

Later on, we identified that all other generic assets in /misc of Drupal core should be understood as libraries as well, as they do not belong to a certain module and they also have interdependencies between them.

However, previously, those generic assets were not really treated as libraries, just generic assets. Therefore, they were loaded with the default JS/CSS weight of module assets. Contrary to that, libraries like jQuery and jQuery UI are loaded with a special low weight, in order to ensure that they are loaded first, so other loaded assets can safely rely on them. In current HEAD, some UI functionality is broken, because the generic assets are loaded with a low weight intended for libraries, and thus executed too early.

So while registering those generic assets in /misc as libraries was a good idea and badly needed, at the same time, we introduced (or rather revealed) a non-obvious inconsistency in our terminology and handling of "libraries". It is no longer clear what the difference between a "superglobal" library and a bundle of assets (optionally with dependencies) actually is.

It is also not entirely clear, why those bundle assets are actually broken, since normally, generic libraries should not be too dependent on the file loading order on a page, since the term library itself denotes that we are talking about an asset that is passive; i.e., it does nothing on its own, and only subsequently loaded assets on the page are actually and actively invoking and using it. So that alone smells like a bug in those assets in the first place to me -- or alternatively, perhaps some of the assets in /misc are not really libraries, but "active" code instead, so it's technically wrong to register them as libraries.

Additionally, it doesn't make too much sense for me to have to specify the weight for every single file within a library. Entirely contrary to that, the notion and built-in support of dependencies implies a natural weighing already: If Library A depends on Library B, then Library B needs to have a lower weight than Library A. Therefore, if Library A is loaded and it specifies a weight of 100, then the internally specified weight for Library B does not really matter -- it needs to be 99; i.e., less than Library A's 100. In turn, it only makes sense to have libraries specify a single weight, which should be applied to all individual asset files in the library.

So overall, I think that this issue unintentionally revealed some problems in our design, definition, and handling of libraries, whereas the actual change of this patch/issue is just the trigger, not the cause. However, it looks like we can resolve those revealed problems relatively easily (as long as there is an agreement on the expected behavior).

Given this analysis, I think we should not commit the stop-gap fix in the last patch.

Discussed with sun. I'll post a summary of that discussion, and possibly an updated patch, later today.

I don't think I'll get to a patch tonight, but wanted to share these thoughts. If we have consensus on approach, the patch should be easy.

It's a little odd that we essentially have two classes of libraries now: The "real" libraries (that get weighted like libraries) and then some others which are registered as libraries but aren't quite as generic so they get weighted higher. I can sort of intuitively understand which is which, but that's a little weird. I wonder if there is some criteria for distinguishing these which could be explained in a code comment.

Following up on #42, ideally all JS files would fall neatly into one of two categories:

1. Passive: A namespaced set of functions that don't do anything to the page on their own. The purpose is to provide useful capabilities for active files to invoke.
2. Active: One or more behaviors (i.e., Drupal.behaviors.foo) that actively do something to the page. One could write active code in a JS file that's not a Drupal.behavior, but one shouldn't. Though, of course, we know that people will use all sorts of 3rd party code that does something active, and that isn't written using Drupal.behaviors.

I believe the distinction between these two kinds of files was the original motivation for having a JS_LIBRARY constant vs. a JS_DEFAULT constant. The idea being to first load all JS code that doesn't do anything on its own, but provides namespaces, and then load all active code that can use those namespaces. For example, by loading jQuery.js early, later code can use the "jQuery" variable. And by loading drupal.js early, later code can use the "Drupal" namespace. This also fits with a common understanding of the term "library": pre-written functionality for application code to use.

As pointed out in #42, originally hook_library() was intended for passive files, and therefore, files added that way are put into the JS_LIBRARY group by default. But it turned out that hook_library() is a useful construct for active files too. This issue really drove that forward, but even before this issue, 'vertical-tabs.js' and 'contextual.js' were added using hook_library(), even though they contain active code.

So, our proposal is to do the following:

• Add a 'group' key to entries in hook_library(). If not set, it would default to JS_LIBRARY, as is the implicit behavior already in HEAD. However, it could be set to something else, like JS_DEFAULT, and then this becomes the default for that library's files. As is already the case in HEAD, individual items within the library could have a 'group' key set on them, and this would override the library's default. The idea behind this new library-level 'group' key would be to acknowledge that hook_library() has evolved to being used for both passive and active files, and therefore, HEAD's assumption that JS_LIBRARY is always the appropriate default is no longer correct. Again, while a lot of this evolution of hook_library() occurred with #12, it had already existed well before then, so simply rolling back #12 wouldn't magically make everything clean.
• For every library defined by core, check whether it's passive or active, and if it's active, set its 'group' to JS_DEFAULT. This would include the ones in #40, but also include vertical-tabs.js, contextual.js, and maybe others. This would address David's concern in #41 that there needs to be some sanity and clarity around when each constant is used.
• Document hook_library() to let developers know to set 'group' => JS_DEFAULT for active code (i.e., code with Drupal.behaviors).

Finally, I'd like to point out that reality always hates compartmentalization. Sometimes the distinction between passive and active isn't clean. For example, drupal.js needs to be loaded with JS_LIBRARY, even though it has this line of active code:

$('html').addClass('js');

So yeah, even though we can provide guidelines, and try to be as clean as we can in core, some issues and WTFs will remain.

Thoughts?

I think that's a fine proposal.

Why can't we just fix the hook_library() definitions so the files that are breaking right now are listed as depending on the libraries that they need in order to not break? I'd really rather not introduce yet another sub-layer of libraries at this point in the game.

#44 makes sense in theory. (It's weird that we use constants with names like JS_LIBRARY to set the default weight of CSS files too, but I guess that's a preexisting problem).

However, it doesn't really fix the bug by itself, does it?

If we put both the drupal.vertical-tabs and drupal.collapse libraries in the JS_DEFAULT group in search of consistency, won't we still need to mess with their individual weights anyway to make sure that the vertical tabs JavaScript always runs first?

I think the patch in #40 is a reasonable fix that is targeted to the bug at hand. It's a bit of a stopgap, but Drupal 7 is about to be released - we are in stopgap territory. Can't we just add a code comment to that about the passive/active stuff and defer anything else until later? The code comment could even have a @todo in it :)

Why can't we just fix the hook_library() definitions so the files that are breaking right now are listed as depending on the libraries that they need in order to not break?

There is no actual dependency, though. The collapse.js code doesn't depend on vertical-tabs.js in any way, but the issue is that if both are on the same page at the same time, vertical-tabs.js must run first.

And AFAIK the 'dependencies' key in hook_library() just makes sure the files are there, but doesn't do anything to ensure their order.

#40 takes care of ensuring the correct order most directly, by fixing the weights of the files we care about and leaving it at that.

Ok thanks, I understand now.

I also am perfectly happy with #40 for D7, other than there's some really good information in #42 and #44 that I think ought to be documented in the code somewhere.

There is no actual dependency, though. The collapse.js code doesn't depend on vertical-tabs.js in any way, but the issue is that if both are on the same page at the same time, vertical-tabs.js must run first.

And AFAIK the 'dependencies' key in hook_library() just makes sure the files are there, but doesn't do anything to ensure their order.

Actually, this worries me a lot. Indeed, there is no direct dependency between those two asset bundles, but if both happen to be loaded on the same page, one needs to come before the other.

Now multiply that problem space with an arbitrary amount of contributed modules and themes.

Let's start simple, and just consider a single additional and hypothetical superimplode module. If we now go ahead and redefine:

$libraries['vertical-tabs'][...]['weight'] = -1;
$libraries['drupal.collapse'][...]['weight'] = 0;

then that module might do nothing special, so the defaults are applied:

$libraries['superimplode'][...]['weight'] = 0;

but if that module's library might also need to run before collapse, but after vertical-tabs, then it would have to specify:

$libraries['superimplode'][...]['weight'] = -1;

However, the actual resulting order of loading and execution depends on which library is attached first and similar black magic. In turn, the actual result can easily be:

$libraries['superimplode'][...]['weight'] = -1;
$libraries['vertical-tabs'][...]['weight'] = -1;
$libraries['drupal.collapse'][...]['weight'] = 0;

Regardless of which actual numbers we will define here, this problem will always exist (unless we'd have a notion of "soft dependencies" that would be automatically figured out prior to page rendering).

To overcome this problem, superimplode module has to implement hook_library_alter() or hook_js_alter() to analyze the weights of all assets and make sure that the superimplode assets always come after vertical tabs and before collapse. Right now, it would have to analyze the group+weight of every single file in the other libraries, in order to adjust the group+weight of every single file in its own superimplode library.

Just thinking aloud.

The problem of a module needing a file between two other sequentially weighted files has existed at least since D5, I think. AFAIK, weights are not restricted to integers, so said module can specify its JS file's weight at -0.5. I agree that as the number of modules out there grows, weights becomes a poor vehicle for managing order dependencies, both for JS/CSS, and for module hooks. In D7, we finally have a hook_module_implements_alter() for hook order management, though in practice we have yet to see how well it works for contrib authors. I think a great goal for D8 would be to manage dependency ordering better, allowing modules to specify what they require to be before and what they require to be after, rather than having to pick some weight number.

That said, I think #40 is an improvement over HEAD in that it fixes a known regression introduced by #12, so back to CNR. When I have a chance, I will post a patch for #44, so we can debate its merits further. Quite possibly, given we're in RC territory already, that patch will be cut down to docs only, but we'll see: maybe some actual code changes will be seen as desirable.

During RC phase, we don't have a great deal of flexibility to sit around with HEAD in a broken state. So committed #40 to HEAD to get us out of it.

I also think that it's much too late for us to be doing the kinds of things that are being talked about here in D7. So I'm marking this back to fixed. If someone comes up with some kind of elegant, non API-changing solution to this we can look at it, but these limitations of weights are well-known going back centuries in Drupal time. I'm not overly fussed about it, personally.

Another regression: #988930: Sticky table headers need to react properly to "show/hide weights column" link
Nevermind. Apologies.