Compress JS aggregation

Updated, clean up a few spaces and fix help text, thanks to kkaefer.

Updated patch which:

- Consolidates _drupal_add_js() and drupal_add_js() to speed things up
- Changes is_null() checks to isset()
- Slight cleanup

Ideas from: http://drupal.org/node/118094 and talking with kkaefer on IRC

Updated patch: fix update.php which incorrectly sets TRUE when it should be FALSE not to cache the JS files.

If cache == FALSE, don't preprocess the JS file.

subscribing. +1 for this functionality... we have one site with > 10 JS includes and this would really help performance.

Missing array structs for header scope, thanks kkaefer.

Damn, parse error.

subscribing
hope to have time to test tomorrow (or over the weekend...)

Can this patch be applied against a 5.1 installation?
If so, I will give it a try a bit later today.

Thanks! Once this one is part of core, we're gonna have a REALLY speedy drupal! Can't wait :D

jacauc

It should in theory apply to Drupal-5 too... I hope to backport this if not after it goes in as well.

With this patch, I get the following two errors on every page:

Fix those index warnings.

+1.. this will be great.

i'll test it a little later.

Applied the patch to a windows XAMPP development box with drupal 5.1
Works very well from what I can see.

Did anyone try to run update.php after applying the patch?
When I click the "update" button on the op=selection page, I get another page with:

"Updating

Please wait while your site is being updated."

...and nothing else.. No progress indicator, and no links back to the main page. - Might be unrelated to this patch, but seems fishy.

Thanks again
jacauc

Also seems to break dynamicload's functionality (part of jstools module) :S

Good catch!

Here's an updated patch which corrects the order of non-preprocessed and preprocessed files.

This one was just a matter of time after the CSS preprocessor went in.
+1 for the functionality.

Been running with the latest patch in the thread, and for some reason I am still not extremely comfortable with it.

I'm not sure if the slight instability can be caused by other JS files, and I cannot put my finger on exactly what's wrong.

Firefug does, however show this from time to time:

jQuery is not defined
mydomain/files/js/626cde960cd17e4e941cf785151f5eb0.js
Line 2
[Break on this error] eval(function(p,a,c,k,e,d){e=function(c){return(c

as well as an error with quicktags module:

edToolbar is not defined
[Break on this error]

help! I broke the thread and can't edit my own posts to add wrappers

>

jacauc, can you post more clear feedback? Like open up that JS file and see what's in there -- jquery and drupal.js and the other files should be in there. Check the cascading JS order to see if things are in the right order.

And make sure your are *not* adding JS files directly in your theme--that could break things if not added correctly.

thanks m3avrck, I will re-enable the JS preprocessing and submit the information, unfortunately I am in a bit of a hurry now, so I can only do it by tomorrow.

Thanks,
Jacques

Here's an updated patch which fixes an issue when preprocessing is enabled but for some reason the files can't be created.

Also, this patch is from a Drupal 5 branch---which I tested very thoroughly with the JSTools module, no problems whatsoever.

This patch also applies to HEAD but is kind of useless to test against right now :-)

+1 for this. (subscribing)

Kinda lost track of this issue.
I applied the patch at #24 on the 22nd and haven't had any issues since.
Great work! Works wonderfully!

Patch works as advertised. Well done.

I'd like StevenW to review this. He knows the CSS aggregation patch inside-out and is probably best placed to spot inconsitencies in the JS aggregation patch.

Not sure if it's this patch that's wrong or the Quicktags module, but quicktags stops working once JS aggregation is switched on.
I have both running on http://www.dieinter.net
...Apart from that, all is ok!

Haven't test, but +1 for the concept. Such functionality directly addresses Drupal scalablity issues.

Nice work!

I've applied and tested on 5.1. The patch works as designed in most cases. Core Drupal behaviours (autocomplete, collapse) are unaffected and work as expected. Contrib javascript behaviours in Javascript Tools package (activemenu, etc.) also work as expected.

I did get one error: tinyMCE (I'm running a patched version of the 5.x dev release) generated this error in Firefox: tinyMCE.baseURL has no properties. I looked at the aggregated script and it was formed as expected, with the scripts in what appeared to be the correct order.

I had no bright ideas of why this is an issue. Likely we'll just need to encourage developers using javascript to test and, if errors arise, call drupal_add_js() with the $preprocess argument set to FALSE.

How much gain do we get from this patch? Clearly it significantly improves a given page load. However, without compression, the obvious advantages of fewer files loaded could possibly be offset by increased overall download size. Since we get different js file combinations on different pages, a given js file is included downloaded to the client once per page visited with a unique combination of js files, rather than once total (assuming browser caching). So, aggregation will always improve performance on the first page loaded, but may increase downloads on subsequent pages.

In my case, after browsing around awhile (as admin), I had only three different files in by files/js cache. This suggests to me that unique combinations may often be relatively few so the advantages of the patch should outweight the costs of increased downloads.

And to the patch itself. It follows the logic of the css aggregation closely, so in general it shares the merits of that patch, including the improvements made through reviews and patching. So it's starting off at a fairly mature state.

Looking over the code I found a few places for improvements, most of them minor, which I've made in the attached patch.

1. This appears to be a fix of an existing issue (wrong 5th argument in the drupal_add_js() call).
   <?php
-  // Prevent browser from using cached drupal.js or update.js
-  drupal_add_js('misc/progress.js', 'core', 'header', FALSE, TRUE);
-  drupal_add_js('misc/update.js', 'core', 'header', FALSE, TRUE);
+  // Prevent browser from using cached progress.js or update.js
+  drupal_add_js('misc/progress.js', 'core', 'header', FALSE, FALSE);
+  drupal_add_js('misc/update.js', 'core', 'header', FALSE, FALSE);
?>

   I guess it makes sense to correct the issue rather than repeating it. For consistency, I've made the same change to another existing drupal_add_js() call in update.php.

2. I've consolidated the several variables for the no_preprocess scripts into one array, saving repeated code.
3. Rearranged some code in drupal_get_js() to apply only when relevant.
4. '#default_value' => variable_get('preprocess_js', FALSE) in the configuration form in system_performance_settings() should default to 0 rather than FALSE. Made this change and (again following the "correct instead of duplicating an error" logic) also made it in the preprocess_css setting.
5. '#title' => t('Aggregate and compress JS files'),, we don't yet compress so for now it should be just 'Aggregate JS files'.
6. if (!$info['preprocess'] || !($is_writable && $info['preprocess']), the second part of this test seems to contain redundancy, we don't need to know if $info['preprocess'] is TRUE if we already know it's not FALSE.
7. Surplus spaces had been introduced in a few places.
8. There was also one more pressing issue. The patch removed _drupal_add_js(), instead adding the two required js library files (jquery.js and drupal.js) straight into a static array in drupal_add_js(). This meant that javascript would be included on every page. We need to restore the original logic that adds these files the first time drupal_add_js() is called. I've done this by making the adding of the array conditional on their being data passed in, as in the original drupal_add_js().

This patch is getting close. Since I've made a number of changes (albeit mostly minor), further review would be useful.

"Likely we'll just need to encourage developers using javascript to test and, if errors arise, call drupal_add_js() with the $preprocess argument set to FALSE."

Does this mean that if a module stops working as a result of this patch, the problem is likely with the module, and not with this patch?
Shall I create a bug report for the problems with Quicktags in it's queue?

Thanks
jacauc

Nedjo, awesome review, I agree with said points.

I'm not 100% sure why the patched TinyMCE would fail -- I wouldn't say that is an indication of this patch but perhaps buggy TinyMCE (which we are trying to fix, heh). One possibility might be that TinyMCE is missing an ";" and when the JS is merged something is on the same line and you get an error.

In terms of compression, I would love to add compression, but a simple regex like CSS won't suffice.

Should be include a hook_compress_js() in there for 3rd party compression engines (see top of post)? Or is someone crafty enough to build a regex? That, I'm not sure about :-)

Would this patch negate the need for the tinymce compressor - and or - is the issue with tinymce possibly related to that (e.g. it's being double compressed)?

Would this patch negate the need for the tinymce compressor - and or - is the issue with tinymce possibly related to that (e.g. it's being double compressed)?

In my experience, the tinymce compressor is broken all on its own. It prevents the link-editor popup from appearing, or at least it did when I last used it.

There are 3rd party JS compressors, like the one jQuery uses. I think stubbing that out to a hook so that someone can write a module that just uses whatever it is jQuery uses (which seems decently stress-tested) or something else if they prefer makes sense. Just eliminating the extra HTTP traffic is already a big win.

We currently ship a packed .js file with core. It would be great for testing and development purposes if we could ship unpacked js files with core, and pack them with a preprocessor all together. jQuery uses Dean Edwards' packer.

Ok so it turns out there's a PHP port of Dean Edwards' packer, but it's OO and only runs on either PHP4 or PHP5, but not both.

So, I refactored the code, threw out some unnecessary bits and generally made it more commented, more readable and more Drupalley. It is licensed under the LGPL originally, so we can relicense under GPL no problem for core.

I think compressing our JS is a logical step after compressing our CSS, and helps keep our original .js files clean and readable.

It seems to work fine for me. For all the .js files I tried, the output is identical to the original PHP version, and almost identical to the JS version (but only because the order of keywords can differ slightly).

When using this, we need to see whether it is more beneficial to compress all JS after aggregating it, or before.

*grmbl*

Some more comments and a tiny bug fix.

Bah. External code has cooties (replacing tabs with spaces).

Subscribing.

Subscribing to issue.

Here's a patch with Steven's packer included. Also cleaned up a problem with Nedjo's patch that duplicated some variable checking (faster now).

Note, compression is broken. See line in common.inc where it's commented out. With packing on you get a JS error.

#24 works great with DRUPAL-5! Is it going to be included officially?

No, no features will be added to Drupal 5. It will be added to drupal 6. And the patch in #44 is the way to go.

I'm not convinced that we need the packer stuff. As soon we add gzip/zip compression, the advantage of the packing might be irrelevant. The packer script adds a whole lot of code that does a whole lot of work for what might provide us a tiny benefit. I think we should test/benchmark this before we can evaluate the usefulness of the packer. In the mean time, I'd prefer us to focus on the aggregation part (without the packer) so we can include that in core already.

Dries: I assumed the dean edwards packer worked well, but it seems there are some minor problems. There could also be bugs in my port of course.

I suggest we drop the hard packer and just leave the simple whitespace/comment compression in there. This mimics our CSS aggregator and is quite fast to do.

I agree here - the JS packer is a lot of code to maintain (and complexity to bugfix) with very little gain, considering that the very vast majority of browsers are happy with gzip nowadays.

I think the admin interface will need some work when we add gzip to the equation, but I don't think that will be hard.

I have reviewed and tested this patch (although not extensively) and it looks good to me. I think we are ready to update to D6 and RTBC.

I agree here - the JS packer is a lot of code to maintain (and complexity to bugfix) with very little gain, considering that the very vast majority of browsers are happy with gzip nowadays.

I think the admin interface will need some work when we add gzip to the equation, but I don't think that will be hard.

I have reviewed and tested this patch (although not extensively) and it looks good to me. I think we are ready to update to D6 and RTBC.

I agree here - the JS packer is a lot of code to maintain (and complexity to bugfix) with very little gain, considering that the very vast majority of browsers are happy with gzip nowadays.

I think the admin interface will need some work when we add gzip to the equation, but I don't think that will be hard.

I have reviewed and tested this patch (although not extensively) and it looks good to me. I think we are ready to update to D6 and RTBC.

Sorry for the dupes - I was getting "500 Internal Server Error"s...didn't realize it was getting submitted.
Someone might want to check the d.org logs around the time of my post to see what was going on :)

OK, let's re-roll the patch without the complex packer, and then we can worry about further improvements in follow-up patches.

subscribing

Ok here's a patch that adds in the minimal JS compression (removing whitespace, same as JS).

Also it fixes the JS files so that they use correction notation, e.g.,:

Need that ";" when you define variable functions like that.

Note, you only need those *optional* semi-colons when you're compressing (e.g., removing whitespace) from JS files, it prevents variables from running into each other by being defined on the same line.

There is still packer stuff in that last patch. Let's just focus on aggregation. (The patch sometimes incorrectly calls this 'compression'.)

Currently the drupal_add_css() both aggregates and compresses CSS (stripping white space).

The proposed drupal_add_js() does exactly the same. However, in this case, the compression is slightly more complex because you can't just remove whitespace and this uses some fancy regexes to clean this stuff out.

I would like to keep these 2 functions consistent.

Now for those talking about gzipping, that is fine, but that is entirely different issue. And not every webserver has gzip support anyways--so fallling back on some basic compression is a good trade off.

I'd like to hear Steven's thoughts on these regexes to clean out spaces and see if he has any ideas to tidy it up more.

Whitespace compression of javascript is a lot more complicated than CSS because you need to deal with operators and strings and some fuzzy end-of-line handling. The multi-regexp engine is IMO a brilliant piece of code (written by Dean Edwards, I just ported it) and works great. Yes, it's complicated, but it does its job. Nobody is asking you to dig in and maintain it.

Ted suggested renaming and moving the functions for the multi-regexping to a generic drupal function. It's certainly a useful mechanism to have around, and we've had quirkier things in core than that (vancode, nested sets, etc). When appropriate, a highly technical solution can be acceptable. Also, as this is only used for creating a cached file, the performance is not that critical either (though it's still quite fast, as the bulk of the work is done by PCRE, not PHP).

Also, just like the CSS compression, removing whitespace and comments is always a good thing. It removes data from the file, so even if you gzip afterwards, you'll still have a smaller file size. Gzip does not realize that it does not need to care about whitespace and comments and will reproduce them faithfully.

Updated patch against DRUPAL-5.

Yes, this is not against HEAD since it's been too hard to test against HEAD without some certain functionality working.

seems like we are close here. just need a proper reroll, no? IMO, we gain a ton still if we leave whitespace.

m3avrck's patch in #60 fails for me on D5.1.

subscribing

Subscribing.

I haven't read the discussion on this issue (sorry).. but I think this is relevant.

I just read on Ajaxian about a PHP based CSS and JavaScript compressor that takes a smart approach. It uses mod_rewrite to run all JavaScript files (could also be CSS) through a PHP script that returns a compressed version of the script.

I imagine this could be done in Drupal using a simple menu callback, allowing even themers to compress any JavaScript they wish to add. If you already have a compression function in place, the callback could simply provide access to it for themes (not just modules). There would have to be some checks on what files are passed to the compression callback, maybe only allowing for files within the base path of Drupal. Shouldn't be too hard with mod_rewrite?

Might be interesting to you, maybe not.. Just letting you know of my findings this morning :)

http://ajaxian.com/archives/jscsscomp-javascript-and-css-files-compresso...

Hm - what would be the advantage of that over the current css compression model (creating compressed files and storing them in the files directory)?
Directly loading the already cached files seems to be faster than running through a php script for each request.

@ximo - I already have a patch (pre 5, but still good) that does exactly that - see http://drupal.org/node/101227

@Frando, the current 'compression' is just whitespace removal, which is fine (and easy), but doesn't come close to gzip in terms of efficiency (there are benchmarks in my issue above). However to server gzip with the correct headers, and without bootstrapping Drupal you need to pass it through a short php wrapper script.

Here is a #60 patch updated to HEAD.

The aggregation seems to be basically functional, however there are two bugs:

1. When JS aggregation is disabled the following PHP warning appears on the user/*/edit page:
   
       * notice: Undefined index: type in /home/LCO/obarton/workspace/drupal-head/includes/common.inc on line 1740.
    * notice: Undefined index: in /home/LCO/obarton/workspace/drupal-head/includes/common.inc on line 1740.
    * notice: Undefined index: type in /home/LCO/obarton/workspace/drupal-head/includes/common.inc on line 1740.
    * notice: Undefined index: type in /home/LCO/obarton/workspace/drupal-head/includes/common.inc on line 1740.
2. The following Javascript error occurs when JS is aggregated and whitespace removal ($script = _packer_apply...) is commented out - it seems the Drupal.settings is not being populated. There is a different JS error when whitespace is removed, but that may just be a side-effect of the main error.
   
   Drupal is not defined
[Break on this error] <script type="text/javascript">Drupal.extend({ settings: { "teaserButton": [...
default.head (line 9)
Drupal.settings has no properties
[Break on this error] var body = $('#'+ Drupal.settings.teaser[this.id]);
8b9999412b1868e99... (line 614)

Can someone please add a big red 'DID YOU FORGET YOUR PATCH?!!' message to project module? ;)

Patch doesn't apply anymore!

@wim - are you sure? I only rolled this yesterday, I can't believe it already doesn't apply!
Perhaps you were mistakenly trying to apply it to Drupal 5 rather than HEAD?

@Grugnog2: I was amazed too... but I did a fresh checkout from HEAD, copied it to my local webserver and then applied the patch. I just repeated this process and had the same results. I may still be doing something wrong, but in that case, I have no clue what that could be... (and no, this isn't the first time I apply a patch).

This is the output btw:

there are no error messages in that output. the patch applied cleanly. the offset is not a problem.

Sorry for the confusion. I repeated the exact same process and now it works... no clue what I did wrong the first time. And indeed no errors were reported, but the files weren't changed whatsoever :s. Sorry again!

In my tests, the only broken feature seemed to be the collapsible fieldsets. They would not collapse by default, nor would they show the collapse-arrow. Note that this only happened for fieldsets, not for unordered lists (so the menu worked just fine).
I hope I didn't make any mistakes this time....

Subscribe

subscribing

Any status on this?

The patch no longer applies. Probably not a big surprise with how long ago it was and how much the code is in flux.

This is a re-roll against HEAD. Hope to get this into drupal 6

i gave it the once over and here's what i noticed.

over all you've got several whitespace changes that add trailing whitespace. i'd like to see those removed.

common.inc
i don't think the _packer_*() functions are well named. they deal with javascript so they should be named as such.

system.module
you add in a t(). which is helpful but might be better off in a separate patch....

why do you change this?

See point #4 in my comment #31, above. The 0 is needed to make the checkbox default value work. FALSE is not recognized as a valid default value.

I'm not sure it was best to add the t() in and change the variable_get 'preprocess_css' from FALSE to 0 in this patch. It might be out of scope for this patch but if there are no objections I'll keep it rolled in with this one. m3avrck is the one that rolled them in there.

The _packer_*() functions were named by m3avrck (I think). Is there something more appropriate they should be named?

Here's an updated patch which fixes a host of issues... now working with the new batch.js, new teaser.js, and removes a bunch of notices and fixed a couple things.

The 0 instead of FALSE is to fix the setting not loading correctly.

@drewish, as for _packer_*() names, Steven named these himself in a previous patch.

While they apply to just JS right now, multi-regex engine can be used in a variety of other cases.

Reviewed on a fresh copy of head without any problems. Looks good to me.

mfer, thanks for your work on this.

Could you please give details on how you tested this? Which specific Javascript behaviours did you test? (There is now a growing list of js effects, some of them only available on specific pages.) At this point, we need to know exactly what works so we can move this to RTBC.

Here's what I tested.

- /modules/comment/comment.js remembering anonymous commenters info
- teaser splitter
- collapsible regions
- text area resizing
- autocomplete
- uploader
- tableselect.js
- progress.js

These all worked.

Color module on the other hand broke. :-(

Tested in Firefox and Safari. I haven't had the chance to test it in IE.

Did I miss anything?

Updated patch to work with color module...

It might be good to also test a batch processing, for instance
-update.php (I don't now if JS aggegation is supposed to happen there ?)
- .po file autoimport (enable a language, install a module with a po file for this language - devel.module for D6, for instance ?)

I can try to test this myself, but this won't be before a day or two...

re-tested everything from #86 and they still work.

Tested color module and it works.

Added a few updates to system module, for testing purposes, and tested it against update.php. That worked as well.

Did all the testing with firebug on and no errors were reported.

Is there any other part that needs testing?

@yched - I tested the auto .po file importing exactly as you suggested with devel module in #88. It worked.

Anything else to test on this?

@mfer : cool - These tests should be sufficient on the 'barch' side. Thanks a lot :-)

Patch applies cleanly, retested everything in #86 with success.

RTBC I think!

Lets try this one more time. We forgot system.js. This patch includes those optional semicolons for system.js

Are there any others? I hunted through the modules and misc directory and think they are all covered now.

Unless there is another javascript file that needs the optional semicolon this should be RTBC. But, I looked through misc and all core modules and it looks like we got them all.

Yes, this should be RTBC now.

Not being a Javascript die-hard, I'd like to get Steven's blessing for this updated patch.

So, will every module we install that uses Javascript have to be altered to take into account things like optional semi-colons if it's going to work with the preprocessor?

Has a list been compiled of all the types of things that have to be changed?

All Modules that would like to take advantage of the preprocessor will need to use those optional semicolons. Other than that there will not need to be other changes and it will be documented.

Please also consider adding code similar to this: http://drupal.org/node/146855 to the JS aggregator, so that changes to JS files will be detected automatically.

Gribnif, we can add in the automatic JS detection later, that is outside the scope of this patch and not tested.

And yes, optional semi colons are required by jQuery plugins since their system uses the same compression engine.

@m3avrck: Aside, do you have an idea how to keep a copyright line intact? The same problem is inside CSS... but for JS it's easier o give examples... like the very popular "jscalendar". i know we'd like to get the file as small as possible, but this will be an issue.

When you add the JSCalendar code simply set it to *not* compress and it'll be intact :-)

Or, you can wait a few till my GPLed calendar picker makes it into CCK ;-)

@m3avrck: jscalendar was only an example... it should be an important feature to keep copyright intact or add a custom copyright line on compress. I'd like to compress and add a copyright line... jsmin have such a feature, too. maybe we are able to add a regex if a line starts with special string it will be moved to the top of the file!? like the @import lines in CSS aggregate.

aside, why not adding and additional parameter to drupal_add_js that is the optional copyright notice if file going to be compressed? looks cleaner then an inline search for specific strings.

ups, last line was outdated... so this should do the trick.

Copyright parameter is out of the scope of this issue. I don't want an extra parameter / debate to hold up this patch, that can always be added later as a bug fix.

patch no longer applies to HEAD

updated to HEAD, again.

i tested latest patch and is working for me.

I really hope this gets in before code freeze.

The current patch does *not* work with the JS shortcut of using ";;" in for loops, I put in a ";true;" for color module, but after talking with Steven, this is a hack for core and shouldn't be in there. However, to get this to properly work requires some recursive looping to protect against this extremely minor case.

Rather than adding in a lot of code and potentially more bugs, I have removed the offending regexes:

With these removed, drupal_pack_js() now safely supports the JS ";;" shortcut in for loops.

The compression savings of the above is only valid in cases where a developer puts in more than one ";" which should happen almost never :-)

tested. works.

RTBC! I can't believe it. Can someone backport this to D5.1?

+1. Backport is sorely needed. PLEASE !!!

I'll post a D5 patch after this goes (waiting for any changes to this patch if any before that happens).

I've sponsored m3avrck to make a D5.1 backport, so it will come. As he says, we'll have to wait for this patch to stabilize first.

The description for the form items on the settings page is silly. For one, the CSS and JS item descriptions repeat 99% of the text, drowning out the few different bits. For another, they're both extremely detailed and longwinded, and just a longer version of what is already said above.

The labels for the settings are also inconsistent. One says "aggregate and compress", the other says "aggregate", even though they both do both. 'JS' is a geek acronym.

There's also a typo in the settings page description.

Finally, I think the phrasing