Just like CSS, but for JS.
Note, there isn't a simple way to compress JS files. The best ways are external scripts like:
JSMin: http://javascript.crockford.com/jsmin.html
Packer: http://dean.edwards.name/packer/
So I propose we allow for a hook_js_pack() in there where it says compression and an external module could make use of either of these styles.
Otherwise, have it and test away.
This is *phase 1* adding in JS aggregation.
Phase 2 will support remixing both CSS and JS with new properties and consolidation -- too heavy to do both JS aggregation & remixing in the same patch.
Comment | File | Size | Author |
---|---|---|---|
#135 | packer-5.x-0.1.zip | 9.62 KB | caktux |
#126 | js-aggregation-119441-126.patch | 696 bytes | bjaspan |
#125 | works.js_.txt | 70.88 KB | bjaspan |
#124 | 19fe50941bbc66948276bf95184f0522.js_.txt | 70.15 KB | bjaspan |
#111 | ts_9.patch | 32.65 KB | m3avrck |
Comments
Comment #1
m3avrck CreditAttribution: m3avrck commentedUpdated, clean up a few spaces and fix help text, thanks to kkaefer.
Comment #2
m3avrck CreditAttribution: m3avrck commentedUpdated 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
Comment #3
m3avrck CreditAttribution: m3avrck commentedUpdated patch: fix update.php which incorrectly sets TRUE when it should be FALSE not to cache the JS files.
Comment #4
m3avrck CreditAttribution: m3avrck commentedIf cache == FALSE, don't preprocess the JS file.
Comment #5
webchicksubscribing. +1 for this functionality... we have one site with > 10 JS includes and this would really help performance.
Comment #6
m3avrck CreditAttribution: m3avrck commentedMissing array structs for header scope, thanks kkaefer.
Comment #7
m3avrck CreditAttribution: m3avrck commentedDamn, parse error.
Comment #8
Owen Barton CreditAttribution: Owen Barton commentedsubscribing
hope to have time to test tomorrow (or over the weekend...)
Comment #9
jacauc CreditAttribution: jacauc commentedCan 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
Comment #10
m3avrck CreditAttribution: m3avrck commentedIt should in theory apply to Drupal-5 too... I hope to backport this if not after it goes in as well.
Comment #11
webchickWith this patch, I get the following two errors on every page:
Comment #12
m3avrck CreditAttribution: m3avrck commentedFix those index warnings.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commented+1.. this will be great.
i'll test it a little later.
Comment #14
jacauc CreditAttribution: jacauc commentedApplied 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
Comment #15
m3avrck CreditAttribution: m3avrck commentedComment #16
jacauc CreditAttribution: jacauc commentedAlso seems to break dynamicload's functionality (part of jstools module) :S
Comment #17
m3avrck CreditAttribution: m3avrck commentedGood catch!
Here's an updated patch which corrects the order of non-preprocessed and preprocessed files.
Comment #18
BioALIEN CreditAttribution: BioALIEN commentedThis one was just a matter of time after the CSS preprocessor went in.
+1 for the functionality.
Comment #19
jacauc CreditAttribution: jacauc commentedBeen 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
Comment #20
jacauc CreditAttribution: jacauc commentedas well as an error with quicktags module:
edToolbar is not defined
[Break on this error]
...update....Defnitely breaks some modules like quicktags and dblclick. A lot of complaints from firebug
Comment #21
jacauc CreditAttribution: jacauc commentedhelp! I broke the thread and can't edit my own posts to add
wrappers
Comment #22
m3avrck CreditAttribution: m3avrck commented> -- had a closing tag issue ;-)
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.
Comment #23
jacauc CreditAttribution: jacauc commentedthanks 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
Comment #24
m3avrck CreditAttribution: m3avrck commentedHere'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 :-)
Comment #25
magico CreditAttribution: magico commented+1 for this. (subscribing)
Comment #26
jacauc CreditAttribution: jacauc commentedKinda 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!
Comment #27
BioALIEN CreditAttribution: BioALIEN commentedPatch works as advertised. Well done.
Comment #28
Dries CreditAttribution: Dries commentedI'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.
Comment #29
jacauc CreditAttribution: jacauc commentedNot 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!
Comment #30
Caleb G2 CreditAttribution: Caleb G2 commentedHaven't test, but +1 for the concept. Such functionality directly addresses Drupal scalablity issues.
Comment #31
nedjoNice 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.
drupal_add_js()
call).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.drupal_get_js()
to apply only when relevant.'#default_value' => variable_get('preprocess_js', FALSE)
in the configuration form insystem_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.'#title' => t('Aggregate and compress JS files'),
, we don't yet compress so for now it should be just 'Aggregate JS files'.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._drupal_add_js()
, instead adding the two required js library files (jquery.js and drupal.js) straight into a static array indrupal_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 timedrupal_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 originaldrupal_add_js()
.This patch is getting close. Since I've made a number of changes (albeit mostly minor), further review would be useful.
Comment #32
jacauc CreditAttribution: jacauc commented"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
Comment #33
m3avrck CreditAttribution: m3avrck commentedNedjo, 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 :-)
Comment #34
Caleb G2 CreditAttribution: Caleb G2 commentedWould 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)?
Comment #35
Caleb G2 CreditAttribution: Caleb G2 commentedWould 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)?
Comment #36
Crell CreditAttribution: Crell commentedIn 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.
Comment #37
Steven CreditAttribution: Steven commentedWe 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.
Comment #38
Steven CreditAttribution: Steven commentedOk 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.
Comment #39
Steven CreditAttribution: Steven commented*grmbl*
Comment #40
Steven CreditAttribution: Steven commentedSome more comments and a tiny bug fix.
Comment #41
Steven CreditAttribution: Steven commentedBah. External code has cooties (replacing tabs with spaces).
Comment #42
Morbus IffSubscribing.
Comment #43
Dave ReidSubscribing to issue.
Comment #44
m3avrck CreditAttribution: m3avrck commentedHere'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.
Comment #45
umonkey CreditAttribution: umonkey commented#24 works great with DRUPAL-5! Is it going to be included officially?
Comment #46
Frando CreditAttribution: Frando commentedNo, 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.
Comment #47
Dries CreditAttribution: Dries commentedI'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.
Comment #48
Steven CreditAttribution: Steven commentedDries: 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.
Comment #49
Owen Barton CreditAttribution: Owen Barton commentedI 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.
Comment #50
Owen Barton CreditAttribution: Owen Barton commentedI 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.
Comment #51
Owen Barton CreditAttribution: Owen Barton commentedI 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.
Comment #52
Owen Barton CreditAttribution: Owen Barton commentedSorry 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 :)
Comment #53
Dries CreditAttribution: Dries commentedOK, let's re-roll the patch without the complex packer, and then we can worry about further improvements in follow-up patches.
Comment #54
dvessel CreditAttribution: dvessel commentedsubscribing
Comment #55
m3avrck CreditAttribution: m3avrck commentedOk 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.
Comment #56
m3avrck CreditAttribution: m3avrck commentedNote, 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.
Comment #57
Dries CreditAttribution: Dries commentedThere is still packer stuff in that last patch. Let's just focus on aggregation. (The patch sometimes incorrectly calls this 'compression'.)
Comment #58
m3avrck CreditAttribution: m3avrck commentedCurrently 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.
Comment #59
Steven CreditAttribution: Steven commentedWhitespace 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.
Comment #60
m3avrck CreditAttribution: m3avrck commentedUpdated 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.
Comment #61
moshe weitzman CreditAttribution: moshe weitzman commentedseems like we are close here. just need a proper reroll, no? IMO, we gain a ton still if we leave whitespace.
Comment #62
Christefano-oldaccount CreditAttribution: Christefano-oldaccount commentedm3avrck's patch in #60 fails for me on D5.1.
subscribing
Comment #63
Wim LeersSubscribing.
Comment #64
ximo CreditAttribution: ximo commentedI 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
Comment #65
Frando CreditAttribution: Frando commentedHm - 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.
Comment #66
Owen Barton CreditAttribution: Owen Barton commented@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.
Comment #67
Owen Barton CreditAttribution: Owen Barton commentedHere is a #60 patch updated to HEAD.
The aggregation seems to be basically functional, however there are two bugs:
Comment #68
Owen Barton CreditAttribution: Owen Barton commentedCan someone please add a big red 'DID YOU FORGET YOUR PATCH?!!' message to project module? ;)
Comment #69
Wim LeersPatch doesn't apply anymore!
Comment #70
Owen Barton CreditAttribution: Owen Barton commented@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?
Comment #71
Wim Leers@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:
Comment #72
moshe weitzman CreditAttribution: moshe weitzman commentedthere are no error messages in that output. the patch applied cleanly. the offset is not a problem.
Comment #73
Wim LeersSorry 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....
Comment #74
hass CreditAttribution: hass commentedSubscribe
Comment #75
dvessel CreditAttribution: dvessel commentedsubscribing
Comment #76
mfer CreditAttribution: mfer commentedAny status on this?
Comment #77
mfer CreditAttribution: mfer commentedThe patch no longer applies. Probably not a big surprise with how long ago it was and how much the code is in flux.
Comment #78
mfer CreditAttribution: mfer commentedThis is a re-roll against HEAD. Hope to get this into drupal 6
Comment #79
drewish CreditAttribution: drewish commentedi 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?
so it matches this?
I'd be more in favor of the FALSE than 0.
Comment #80
nedjoSee 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.
Comment #81
mfer CreditAttribution: mfer commentedI'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?
Comment #82
m3avrck CreditAttribution: m3avrck commentedHere'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.
Comment #83
m3avrck CreditAttribution: m3avrck commented@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.
Comment #84
mfer CreditAttribution: mfer commentedReviewed on a fresh copy of head without any problems. Looks good to me.
Comment #85
nedjomfer, 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.
Comment #86
mfer CreditAttribution: mfer commentedHere'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?
Comment #87
m3avrck CreditAttribution: m3avrck commentedUpdated patch to work with color module...
Comment #88
yched CreditAttribution: yched commentedIt 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...
Comment #89
mfer CreditAttribution: mfer commentedre-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?
Comment #90
mfer CreditAttribution: mfer commented@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?
Comment #91
yched CreditAttribution: yched commented@mfer : cool - These tests should be sufficient on the 'barch' side. Thanks a lot :-)
Comment #92
Owen Barton CreditAttribution: Owen Barton commentedPatch applies cleanly, retested everything in #86 with success.
RTBC I think!
Comment #93
mfer CreditAttribution: mfer commentedLets 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.
Comment #94
mfer CreditAttribution: mfer commentedUnless 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.
Comment #95
m3avrck CreditAttribution: m3avrck commentedYes, this should be RTBC now.
Comment #96
Dries CreditAttribution: Dries commentedNot being a Javascript die-hard, I'd like to get Steven's blessing for this updated patch.
Comment #97
Gribnif CreditAttribution: Gribnif commentedSo, 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?
Comment #98
mfer CreditAttribution: mfer commentedAll 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.
Comment #99
Gribnif CreditAttribution: Gribnif commentedPlease 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.
Comment #100
m3avrck CreditAttribution: m3avrck commentedGribnif, 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.
Comment #101
hass CreditAttribution: hass commented@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.
Comment #102
m3avrck CreditAttribution: m3avrck commentedWhen 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 ;-)
Comment #103
hass CreditAttribution: hass commented@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.
Comment #104
hass CreditAttribution: hass commentedaside, 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.
Comment #105
hass CreditAttribution: hass commentedups, last line was outdated... so this should do the trick.
Comment #106
m3avrck CreditAttribution: m3avrck commentedCopyright 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.
Comment #107
mfer CreditAttribution: mfer commentedpatch no longer applies to HEAD
Comment #108
mfer CreditAttribution: mfer commentedupdated to HEAD, again.
Comment #109
moshe weitzman CreditAttribution: moshe weitzman commentedi tested latest patch and is working for me.
Comment #110
BioALIEN CreditAttribution: BioALIEN commentedI really hope this gets in before code freeze.
Comment #111
m3avrck CreditAttribution: m3avrck commentedThe 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 :-)
Comment #112
moshe weitzman CreditAttribution: moshe weitzman commentedtested. works.
Comment #113
Christefano-oldaccount CreditAttribution: Christefano-oldaccount commentedRTBC! I can't believe it. Can someone backport this to D5.1?
Comment #114
dkruglyak CreditAttribution: dkruglyak commented+1. Backport is sorely needed. PLEASE !!!
Comment #115
m3avrck CreditAttribution: m3avrck commentedI'll post a D5 patch after this goes (waiting for any changes to this patch if any before that happens).
Comment #116
Wim LeersI'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.
Comment #117
Steven CreditAttribution: Steven commentedThe 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.
Comment #118
Steven CreditAttribution: Steven commentedActually 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.
Comment #119
Christefano-oldaccount CreditAttribution: Christefano-oldaccount commented@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!
Comment #120
webchickComment #121
Christefano-oldaccount CreditAttribution: Christefano-oldaccount commentedAh yes, please excuse me while I go off and grumble at myself. Thanks webchick.
Comment #122
m3avrck CreditAttribution: m3avrck commentedThanks to Wim Leers for sponsoring the backport of this patch to Drupal 5: http://drupal.org/node/149402
Comment #123
(not verified) CreditAttribution: commentedComment #124
bjaspan CreditAttribution: bjaspan commentedI 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.
Comment #125
bjaspan CreditAttribution: bjaspan commentedHere's the working version.
Comment #126
bjaspan CreditAttribution: bjaspan commentedI finally tracked down the problem here. In #55 and 56, m3avrck changed the patch and said:
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.
Comment #127
bjaspan CreditAttribution: bjaspan commentedTwo 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.Comment #128
bjaspan CreditAttribution: bjaspan commentedSee patch in #126.
Comment #129
mfer CreditAttribution: mfer commentedIs 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.
Comment #130
bjaspan CreditAttribution: bjaspan commentedThis 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.
Comment #131
(not verified) CreditAttribution: commentedComment #134
RobLoachRe-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.
Comment #135
caktux CreditAttribution: caktux commentedHere'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.
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 :)
Comment #136
lilou CreditAttribution: lilou commentedSee also #156124: JS and CSS aggregation deletes license information
Comment #137
lilou CreditAttribution: lilou commentedAdd tag.
Comment #138
RobLoachI 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.
Comment #139
markus_petrux CreditAttribution: markus_petrux commentedSubscribe
Comment #140
Wim LeersPlease also see this comment of mine on a duplicate issue: http://drupal.org/node/210447#comment-1579210.
Comment #142
SeanBannister CreditAttribution: SeanBannister commentedSub
Comment #143
catchComment #145
bryancasler CreditAttribution: bryancasler commentednudge
Comment #146
mfer CreditAttribution: mfer commentedWe 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.
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.
Comment #147
markus_petrux CreditAttribution: markus_petrux commentedPluggable js/css processors could resolve this, it would be easy to pre/post process all these resources by contrib/custom modules.
Comment #148
mfer CreditAttribution: mfer commented@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.
Comment #149
no2e CreditAttribution: no2e commentedRegarding #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).
Comment #150
Gaelan CreditAttribution: Gaelan commentedMy 2¢: Why don't we use the web-based version of Google Closure?
Comment #151
Wim Leers#150: We can't rely on that. Google routinely EOLs services/products. Some Drupal sites run without access to the internet. And so on.
Comment #152
RobLoachThe 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.
Comment #153
mfer CreditAttribution: mfer commented@Gaelon there are a few issues here.
This issue isn't just a matter of what can be done but what should be done for the mass of Drupal users.
Comment #154
Wim Leers@mfer what are your thoughts on #149?
Comment #155
mfer CreditAttribution: mfer commented@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.
Comment #156
Wim Leers@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 :)
Comment #157
JohnAlbinTagging
Comment #158
Owen Barton CreditAttribution: Owen Barton commentedNote 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.
Comment #159
RobLoachInstead 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
Comment #160
Wim Leers#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:
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.
Comment #161
mfer CreditAttribution: mfer commented@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?
Comment #162
klonos...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?
Comment #163
Wim LeersI 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 :)
Comment #164
nod_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
won't happen.Comment #165
Wim LeersWasn'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.
Comment #166
nod_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.
Comment #167
klonosIs 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?
Comment #168
hass CreditAttribution: hass commentedI 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.
Comment #169
Wim Leers#168: right. That was the main reason: we cannot guarantee validity. Thanks :)
Comment #170
mfer CreditAttribution: mfer commented@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...
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.
Comment #171
Dave ReidAs 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.
Comment #172
klonosI 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.