Compress JS aggregation

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

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

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

    * notice: Undefined index: footer in /Applications/MAMP/htdocs/head/includes/common.inc on line 1672.
    * warning: Invalid argument supplied for foreach() in /Applications/MAMP/htdocs/head/includes/common.inc on line 1706.

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

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

Apologies if this is not related

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

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

+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).
   

   -  // 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

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.

#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

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

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.

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-compressor

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:

>: patch -p0 < ts_4.patch 
patching file includes/common.inc
Hunk #1 succeeded at 1638 (offset -3 lines).
Hunk #2 succeeded at 1666 (offset -3 lines).
Hunk #3 succeeded at 1675 (offset -3 lines).
Hunk #4 succeeded at 1699 (offset -3 lines).
Hunk #5 succeeded at 1730 (offset -3 lines).
patching file misc/tableselect.js
patching file misc/progress.js
patching file misc/update.js
patching file misc/textarea.js
Hunk #1 succeeded at 28 (offset -8 lines).
patching file misc/upload.js
patching file misc/drupal.js
patching file misc/collapse.js
patching file misc/autocomplete.js
patching file update.php
Hunk #3 succeeded at 451 with fuzz 1 (offset 1 line).
patching file modules/system/system.module
Hunk #1 succeeded at 220 (offset -36 lines).
Hunk #2 succeeded at 701 (offset -25 lines).
Hunk #3 succeeded at 1536 (offset -40 lines).

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?

-    '#default_value' => variable_get('preprocess_css', FALSE) && $is_writable,
+    '#default_value' => variable_get('preprocess_css', 0) && $is_writable,

so it matches this?

+    '#default_value' => variable_get('preprocess_js', 0) && $is_writable,

I'd be more in favor of the FALSE than 0.

+    '#default_value' => variable_get('preprocess_js', 0) && $is_writable,

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?

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?

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.

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.

@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.

@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.

jsmin <fulljslint.js >jslint.js "(c)2002 Douglas Crockford"

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.

drupal_add_js($data = NULL, $type = 'module', $scope = 'header', $defer = FALSE, $cache = TRUE, $copyright = NULL)

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

drupal_add_js($data = NULL, $type = 'module', $scope = 'header', $defer = FALSE, $cache = TRUE, $preprocess = TRUE, $copyright = NULL)

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.

tested. works.

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

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

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 "... only turn [css/js aggregation] on when your site is in production" is subject to misinterpretation. It can sound like "while you are in the process of producing it", suggesting the opposite of what it should.

But since this patch has been in the queue long enough, I fixed all that and pushed the magic button that commits it to HEAD.

Actually there was a tiny bug left, which seems to result from a difference in behaviour between JS regexps and PCRE regexps. The capturing group (\b|\x24) (word boundary or dollar character) is either zero-width or 1-width. This triggers some weird behaviour where the spaces between keywords are doubled.

It's fixable by changing the group to a zero-width look ahead/behind instead.

I tested the fix on various core JS in Safari, Firefox and IE6 without problems. Committed to HEAD.

@steven: I'm not running HEAD and haven't seen the text in the post-patch admin settings, so I agree with you that using the correct wording is very important and defer to you. However, why set status to fixed if the wording is so confusing?

@wim: thank you for sponsoring the backport!

But since this patch has been in the queue long enough, I fixed all that and pushed the magic button that commits it to HEAD.

Ah yes, please excuse me while I go off and grumble at myself. Thanks webchick.

I encountered a problem with the backported-to-D5 version of this patch. I know this patch isn't going into D5 but since (I assume) the JS-compression logic is the same in both D6 and the D5-backport, the problem may/probably also exists in D6. Someone else noticed the same problem with D5 and reported it (http://drupal.org/node/149402#comment-294810).

The error message I receive from JavaScript is "missing ; before statement". This is with FF 2.0.0.7 and 3.0 alpha on Windows. After slowly taking apart the compressed JS I appear to have discovered that the problem is with code like }Drupal.jslink=..., i.e.: close-bracket followed by an assignment with no intervening newline.

I fixed the problem by editing the compressed JS by adding a newline after every occurrence of /}+/ (any positive number of close-brackets) EXCEPT in the compressed jQuery code. The fact that I can't apply this regexp replacement to jQuery means I can't actually use it as a fix, it is just a hand-edited example of how to fix the problem.

I am attaching the original, broken compressed JS to this followup and my hand-modified one to the next one so people more familiar with the patch can figure out the correct solution.

Compressing whitespace out of program code is complex and error prone, and ISTM that the big benefit of this patch is avoiding multiple HTTP requests for all our many JS files. The actual byte savings by eliminating whitespace is probably fairly minimal. Perhaps we should consider not eliminating whitespace.

Here's the working version.

I finally tracked down the problem here. In #55 and 56, m3avrck changed the patch and said:

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

var foo = function bar() {
};

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.

In other words, the JS compressor breaks on legal Javascript. This is a mistake. Requiring valid JS to be changed to be compatible with the compressor is a wrong decision, especially since the $cache argument is TRUE by default (but even if it were not). Oh, and this new requirement is also not documented. It just cost me half a day of work to figure this out and not all JS/module developers will even be capable of grokking the packer compression logic to figure out what is going on. Not acceptable for core.

In my particular case, it was the D5 CCK Link module that has the close-brace not followed by a semi-colon. I'm sure the D6 version will, too, along with numerous other modules, and we should not force them to change.

I've attached a patch which fixes this problem by making sure a newline appears after close-braces.

I predict this is not the last time that this JS compression code will bite us. I recommend removing it completely; just concatenating the files into a single request is the big win of this patch.

Two more notes:

1. I marked this 'critical' because the bug allows valid JS in any enabled module to disable all JS from all modules. It's a pretty severe loss of function and most sites will have no way to debug or fix the problem.

2. @Steven: There is a bug in _packer_apply(). The replacement text (e.g.) "\$0\n" matches the regexp /^\$\d+$/ (because the $ can match a single trailing newline) and so it triggers the optimized replacement method instead of the normal one. The result is that '$0' gets substituted, not "\$0\n". I guess I'll file a separate issue on this.

See patch in #126.

Is this in the D5, D6, or both version of this patch?

If it's in D5 please post it at http://drupal.org/node/149402 where the backport is maintained.

If it's for D6, can you please open up a new issue.

This is a D6 issue and I attached a D6 patch. However, I have now created a new issue for this patch: http://drupal.org/node/183940. I'm returning this issue to 'fixed' (even though it isn't :-)).

I already updated the D5 patch at the location you referenced.

Re-opening issue for a follow up patch to #251578: More flexible js ordering and an alter operation. This issue will build upon it once it is committed to core to compress the aggregated Javascript.

Here's a little module I just created to pack my javascript files using Dean Edward's Packer. By default it only aggregates and packs without using eval (compression setting to 'None' with Packer).

It looks for your files relative to your theme's path.

  echo packer_combine(array('js/scripts.js','js/extras.js'));

It's not much, but it does the job for what I need. I usually strip out $styles and $scripts from the frontend of my websites and use only my css and javascript, keeping Drupal's javascript and css for the admin only. I hope it can help someone, and you're welcome to take it to the next level and make an official module if you want :)

See also #156124: JS and CSS aggregation deletes license information

Add tag.

I think this should be postponed for #352951: Make JS & CSS Preprocessing Pluggable. Marked #210447: Introduce Javascript compression as a duplicate. Note that you should look at #352951: Make JS & CSS Preprocessing Pluggable for some additional great ideas.

Subscribe

Please also see this comment of mine on a duplicate issue: http://drupal.org/node/210447#comment-1579210.

Sub

nudge

We have a couple things we need to overcome before we can move forward on this technically. These are something I don't know how to overcome in core which would have me wanting them in contrib.

The case we need to take for these includes core JS, contrib JS, and custom JS written on custom sites.

1. The JavaScript in Drupal is GPL licensed. Sending JS to the browser to be run is a form of distribution. If we have custom site code and minify it we are sending obfuscated code to the browser not distributed in its full source anywhere. We may be able to get around this by putting a comment in the minified JS file pointing to a place they can get the full source. But, we'll want some people who can speak to the legalities and verify this approach. And, I'm not sure how this works legally outside the USA.
2. The best minifiers are in Java (Google Closure Compiler) and JS (UglifyJS - what jQuery uses). JSMin (which has a PHP port) produces much larger files than either closure compiler or UglifyJS. For example, jQuery 1.7.1 in full source (development version) is 248235 bytes in size. The production version (run through uglifyjs plus a copyright comment in the header) is 93868 bytes in size (when gzipped this comes down to 31KB). But, jsmin only brings the file size down to 139224 bytes. UglifyJS does a much better job. In the world of web performance optimization this difference is huge. But, to use UglifyJS we would need a lot more dependencies for Drupal. To use the jsmin php port we would get weaker minification. This is a problem.

   Back when this issue was started the world of minification was a much different place. We've come a long way and now have much better tools. The landscape has changed.

A good first step we can do now is help every module and theme provide production minified JS.

Note, I am not a lawyer and this is not legal advice.

Pluggable js/css processors could resolve this, it would be easy to pre/post process all these resources by contrib/custom modules.

@markus_petrux there already is an issue for making preprocessing pluggable in #352951: Make JS & CSS Preprocessing Pluggable. This issue is to add minification to core.

Regarding #146 by mfer:

Maybe JavaScript License Web Labels could help? It's a way (proposed by GNU) to enable minification without violating the license.

Therefor you have to create a table with one row per JS file (linking to the minified JS file, the license and the unminified source code).
At the moment a link to the page containing the table is required for each page using JS. But I think this convention could be discussed with GNU, and maybe a solution with a <link> in the <head> would suffice (at least for CMS).

Originally it's meant for people who don't want to execute non-free JavaScript while surfing (agents could check the table and only allow free JS to run).

My 2¢: Why don't we use the web-based version of Google Closure?

#150: We can't rely on that. Google routinely EOLs services/products. Some Drupal sites run without access to the internet. And so on.

The JavaScript world has their own tools to compress their sources. Both jQuery and jQuery UI use npm as their package management system, and grunt.js as their build tool. This kind of build process is getting more and more popular among JavaScript packages.

Compressing on-the-fly is extremely expensive, and these JavaScript libraries already have a working recommended build solution. We should use their build tools and commit the built packages of the files: #1341792: [meta] Ship minified versions of external JavaScript libraries. Either that, or introduce a build process for Drupal that makes use of Composer/npm/grunt.

We want a minimal viable solution here, and this is it. Contributed projects like AdvAgg, CDN and jQuery Update can then step in and do additional things if needed. But shipping the already built versions of these is the way to go.

@Gaelon there are a few issues here.

1. We can't have sites minifying on the fly for all their JS. Shipping JS from a server to a browser is distribution. That means you need to comply with licenses and regard to distribution. For example, Stripping out a license/copyright comment would cause a license to be violated. Dealing with all possible permutations for included scripts is a nightmare.
2. Some sites are behind firewalls, etc. That means they can't send JS out to public APIs. Even if they are EOL (as Wim pointed out) there are data security issues. Making the Preprocessing system pluggable could help because people could swap it out if their use case allowed. But, by default that doesn't work.
3. We are left with the ability to minify core files and ship that (and still follow licenses without setting up Drupal users to violate licenses). The most performant way to handle that for users by default is to ship with minified files. An added benefit is we can use some automation tools to help us find errors. The first serveral versions of Drupal 7 shipped with broken JS that could have been easily detected.

This issue isn't just a matter of what can be done but what should be done for the mass of Drupal users.

@mfer what are your thoughts on #149?

@wim That could potentially be a sane solution. But, I would think lawyers need to look at it and vet it first. Would the Acquia lawyers be ok with a solution like this? I'm going to query the lawyers I have access to and see what they think.

@mfer: Great! I'll figure out who to talk to at Acquia and get it checked out. I'm hopeful though — if GNU put this online, they most likely have vetted it around the world :)

Tagging

Note that there are several issues to do with improving how licenses are handled for JavaScript, including a meta issue at #1649670: [META] Improving Drupal sites JavaScript Licence Compliance. I would suggest that trying to resolve the licensing question on this issue is not ideal, as there are several aspects to consider, and minification is complex enough on it's own. There are also more fundamental JS license issues for sites using Drupal even when not using aggregation or minification - #1649654: Non-trivial JavaScript files need GPL license declaration for compliant distribution to browsers.

I have been talking with folks at the FSF and they are very happy to provide support to us around these issues. In terms of the question above for GPL code, either Web Labels or comment blocks (including appropriately marked up license declaration and source links) are sufficient from the FSF point of view. Inline comments are probably easier (and probably a good idea in any case), but the Web Label approach may be better for some cases - e.g. if we need to prompt the site builder to confirm licenses for specific files. I suggest we continue discussion around licenses on the meta or new issues though.

Instead of aggregating JavaScript together, a much better solution would be to move to an AMD architecture along with Assetic to handle it: #1542344: Use AMD for JS architecture
http://drupal.org/node/1667512
http://drupal.org/node/1667500

This would also help: #1341792: [meta] Ship minified versions of external JavaScript libraries

#155/@mfer: the legal people at Acquia think the opinion shouldn't come from Acquia. The Drupal Association is guaranteed to have no bias, so it's better to consult with them. That being said, they said:

Generally, the GNU folks have good experts thinking about these problems. If GNU has a solution they're happy with, I'd feel highly comfortable with it just because of who it came from - especially because Drupal is using GNU licensing (GPL).

That was also my personal feeling: GNU folks don't tend to publish things they aren't absolutely 100% certain of. If GNU is satisfied with it, why can't we be?

What should we do to move this forward as fast as possible? This is a potential solution to *lots* of issues: #1649670: [META] Improving Drupal sites JavaScript Licence Compliance, #1649654: Non-trivial JavaScript files need GPL license declaration for compliant distribution to browsers, #1537198: Add a Production/Development Toggle To Core.

@wim I spoke to a legal person as well and I'm fine with the stance on GNU (quote you have).

What needs to happen with this issue?

What needs to happen with this issue?

...this issue was initially opened back in 2007!

I tried going through it in order to update the issue summary and help to answer this "what needs to be done?" question but the issue is really really long and was set to "postponed" in #134 back in June 2008. It stayed inactive for half a year from September 2008 till May 2009. After some more inactivity, in December 2009 it was switched to D8 (without lifting the "postponed" status) and stayed inactive yet again for two years this time (not counting the bump in #145 in July 2011). So from December 2011 and sporadically till recently we got some action, but I honestly doubt that what we're talking about now (2012) has anything to do with whatever was discussed so long ago (2007-2009). I honestly think that we should close this issue here and open a new one that we can easily follow. No need to have people interested in this task going as back as 2007 to see what was discussed then and try to figure things out. Right?

I think @klonos is right; this issue is too long already to collaborate effectively.

The first thing that needs to happen in my opinion is implementing support JavaScript Web License Labels. Once that is done, we can implement compression :)

I'm pretty sure on the fly minification won't happen in core at this point. This will most likely be in contrib for D8 thanks to #352951: Make JS & CSS Preprocessing Pluggable. Maybe it'll make it for D9? who knows.

Since the main concern ATM is licencing let's take that to the licence issues #1649670: [META] Improving Drupal sites JavaScript Licence Compliance. The technical details of the implementation can be discussed in a follow-up of the meta issue. It should be part of the jshint, grunt, assetics discussion.

Thanks to all you guys for the input. Putting as won't fix since the original Compress JS aggregation won't happen.

Wasn't licensing the main blocker here? I'd like slightly more explanation as to why this can't happen in D8.

I +1000 having it pluggable, but if Drupal can ship with a default, that'd be better for WPO.

We should ship with minified files to start with, minification takes ages and with the current architecture of how JS files are processed, that would introduce crazy delays for page load when minification happens.

I'd prefer to see what contrib can come up with once the proper pluggable architecture is in place to allow it. We just don't have time to deal with it for D8.

Is there any issue filed for js minification in core (for D9 or future) so as for the idea not to be forgotten? Or is #352951: Make JS & CSS Preprocessing Pluggable supposed to tackle that too?

I remember very well that js minify has been removed from core for the reason that valid js code has been destroyed. Not every lib follows the jquery rules... If we like it or not.

#168: right. That was the main reason: we cannot guarantee validity. Thanks :)

@Wim, the minification setup we had in core at the time was producing broken JS. The tools to minify have gotten better (like UglifyJS and Closure Compiler) but they aren't written in PHP.

We can either...

1. Don't minify.
2. Ship minified files. This will improve performance to the masses of core users. But, contrib needs to manually mange this.
3. Punt to contrib. I have a feeling a lot of Drupal sites won't install these performance improvement modules.
4. Minify on the fly in core. We'd need to add extra dependencies to Drupal.

I'm for shipping with minified files and making it easy for other modules to do so as well. It may be the most cautious but I've learned (from experience) to be cautious here.

As a contrib maintainer I would be completely ok with solution 2 for my modules as well. It would become part of the best practices for release management, just like core. As long as we have the tools or instructions how to do it and it's relatively easy, that's the best solution.

I agree that #170 2 seems like the best solution. We should go ahead and do that + communicate this to contrib as best practice in order to improve performance. If later on we have the tools that do that automatically for us (in packaging or in core on-the-fly), then we can revisit this in order to improve/automate things.

On the minus side of doing this is that it will make it harder to manually apply patches that include changes to .js files - especially for newcomers.