We should ship both minified versions of every third party JavaScript library in core.
This will probably be done as a part of #2203431: [meta] Various asset (JavaScript) libraries have to be updated to a (minified) stable release prior to 8.0.0.
Libraries
- jQuery
- jQuery UI
Proposed solution
Go through third party libraries added in core.
Find a minified version of the library matching each version
Post a patch to switch.
Potential follow-up
Include both minified and un-minified versions in core, and a switch to change between them.
Original report by catch
#1328900: Update to jQuery 1.7 added an un-minified version of jQuery to core to ease debugging during the 8.x cycle. This is a great idea but we need an issue open to make sure that we remember to switch to the minified one prior to release.
Comment | File | Size | Author |
---|---|---|---|
#41 | 1341792-41.patch | 911.92 KB | RobLoach |
#39 | 1341792.patch | 933.21 KB | RobLoach |
#32 | 1341792.patch | 939.99 KB | RobLoach |
#29 | 1341792.patch | 1023.97 KB | RobLoach |
#28 | 1341792.patch | 939.99 KB | RobLoach |
Comments
Comment #1
plachWhat about introducing built-in support to minify our (aggregated?) javascripts? This way we could ship with all the core/contrib libraries full versions, thus making development far easier. At the same time we would have yet another performance improvement available for every included javascript.
Comment #2
catchThat's in #119441: Compress JS aggregation, which in turn is postponed on #352951: Make JS & CSS Preprocessing Pluggable, would be good to do that yeah - although the better minifiers now I don't think you can run in PHP (require server side js or similar) so we may still want to ship pre-minified js.
Comment #3
sunWe have a tag for that. :)
Comment #4
mfer CreditAttribution: mfer commentedFirst, core shouldn't automatically minify JS for a couple reasons. Well two problems. Unless we have a way out of them.
In addition to these two large objections (that I'm really not sure how to get around) there is a difference in the quality of the minifier. JSLint is a rather weak minifier (producing larger files) compared with something like uglifyJS (what jQuery currently uses). I'd like to see us use something smart and strong as opposed to something weak but we can stick it into PHP.
Maybe we should start on some build scripts to automate doing the minification.
Tagging as mobile since the file size issue has mobile implications.
Note, I am not a lawyer. This is not legal advice.
Comment #5
mfer CreditAttribution: mfer commentedI wanted to make a clarification on bullet 2. If the source JS is already distributed through d.o or a libraries website (like jquery.com) then minified code is fine. Site specific code is where we run into a problem. Implement Drupal.theme to change the look of something in a theme and you now fall under the custom code being distributed based on GPL code problem.
Note, I am not a lawyer. This is not legal advice.
Comment #6
Crell CreditAttribution: Crell commentedAs long as the unminified code is still available, that's probably good enough. Strictly speaking GPLv2 requires you to offer original source on CD, but no one actually does that and GPLv3 does away with it. Download is fine. Legally it's no different than image styles, I'd argue.
Plus, I don't see why a minifier couldn't be programmed to leave the first docblock in a file alone and just strip the rest. That would cover the attribution question.
So legally, I am not all that worried, to be honest. The technical capabilities of different minifiers I cannot speak to.
Comment #7
catchThis is the wrong thread for that discussion. We've been shopping minified jquery.js and jquery_ui for years via officially distributed versions with licencse headers at the top. About six weeks ago we put the unminified versions in instead to assist debugging during 8.x development. Before we release, we'll need to switch them back again.
Having minification itself in core is being discussed elsewhere. Possibly we could mark this issue won't fix in case we ever sort that out properly, but that's not what's proposed here.
Comment #8
RobLoachDefinitely have to switch to minified jQuery/jQuery UIs before releases come. When should we do that though? Before alpha? Before beta?
Comment #9
catchHere's the issue for general minification #119441: Compress JS aggregation.
Let's not mark this postponed, it's not waiting on anything except for the release cycle, and yeah we need to discuss when is a good time to do that.
Comment #10
mfer CreditAttribution: mfer commentedWe should ship minified versions of every core JavaScript file. If doing all JS files as minified needs to happen elsewhere I'll open or join an issue for that. We should do it for performance. There's a reason all the libs (like jquery) ship a minified version called the production version. The same reason applies to all our JS.
Comment #11
catchRight, that's in #119441: Compress JS aggregation, but if that doesn't happen, we should not ship with the unminified jquery.js just because that didn't get dealt with during the release, which is what this issue is for.
Comment #12
mfer CreditAttribution: mfer commentedAfter a number of conversations with those around the Drupal community I believe #119441: Compress JS aggregation will fail. There are non-technical issues we have to work out. Because performance is important we ship with the recommended minified version of jquery.js (and some other components like jQuery UI). For the same reason these libraries use minified versions of their libraries in production we should minify all the JS within Drupal for production.
Since doing this will take more thought and work than dropping in a minified version of jquery.js I'm looking to start work on this earlier in the Drupal 8 life cycle. Ideally, it would be nice to backport this to Drupal 7 if possible. If I need to tackle this case elsewhere or we need more justification on why I'd be glad to share any of the details I have.
Comment #13
tstoecklerBecause #1085590: Update to jQuery UI 1.9 will make jQuery UI also un-minified, I added a little list to the issue summary so that we don't lose track of which files to minify: link
Comment #14
RobLoachBTW, might be bikeshedding this way out of proportion, but Assetic has filters to minify/compress JavaScript and CSS.... CssCompressorFilter and JsCompressorFilter allow you to use YUI Compressor. It's also quite easy to extend it to add your own filters.
Comment #15
catchThis issue has absolutely nothing to do with core aggregation or compression. It is about making sure the shipped versions of jquery.js et al. are minified before we release, because (for possibly the first time ever) 8.x currently has un-minified versions to make debugging easier during development.
As mfer points out it's very unlikely we'd want to do that via any possible minification method that could possibly be shipped with core, because none of the better minification methods are in PHP, not to mention the fact that jquery supplies those files for production anyway. Anything else should be discussed in #119441: Compress JS aggregation or one of the several other issues dealing with this.
I'd be fine with opening a separate issue to provide minified versions of Drupal's own javascript (i.e. checked in, not via a runtime pre-processer) (drupal.js etc.) but again, not for this issue.
Comment #16
RobLoachAh, I understand. Thanks for explaining the scope of this issue :-) . Definitely things to think about!
Comment #17
RobLoachComposer has the ability to execute certain scripts post-installation. So, I thought it might be a good idea to use that, along with Assetic, to minify/compress all JavaScript/CSS files. Don't really have the time to do up a patch right now, buuuuuut.....
drush dl composer
drush composer install
Assetic will process all JavaScript and CSS for Drupal core and jQuery UI, and save the .min files. Currently it does not compress them, but that's as easy as adding the filters in...
Comment #18
nod_Having a proper folder for our javascript would help.
core/js/src
with unminified JS (including jquery/ui for debug) andcore/js/min
for the processed files. Core modules can use the same paradigm,js/src
folder and be picked up by the build script. And the build script incore/scripts
to minify all our things.For contrib modules what Rob proposed that would be a good follow-up for the pluggable css and js processing.
Comment #19
mfer CreditAttribution: mfer commentedWe can't simply minify all the JS and CSS automatically. I wish it were that simple. For example, if we automate the minification jquery.ba-bbq.js will have the second copyright/license notice removed. This is not a good thing.
Note, for anyone not familiar with the situation, JavaScript and CSS sent from the server to the browser is distributed. We need to send the copyright and license info even for minified files. We need a process that makes sure this is properly handled.
That said, if we manually handle the external libraries and automate the generation of JS for our core stuff we should be in good shape.
We should also minify module JavaScript as well.
I would suggest using UglifyJS which Assetic can support.
Comment #20
nod_Well if we're allowed to add a little ! to third party libs. in the comments (as in
/**!
) then yuicompressor won't delete the comment block. I'm just hoping other minification tools can work with something like that. It might be an option and an encouraged practice if that's the case.But yes, that's something to pay attention to.
Comment #21
mfer CreditAttribution: mfer commented@_nod YUI compressor, Google Closure Compiler, and UglifyJS each have their own way of handling comments you want to save. UglifyJS generates the smallest minified files and does so the fastest. There is no standard for handling comments you want to save. We would either A) have to modify the libraries we include when we include them (and update them), B) Get the library authors to change their libraries and keep them our way or C) Manually manage the minified files.
I just want to make sure this is handled well up front rather than forgotten or a bolt on after thought. Since this has to do with licenses it can affect all of the big Drupal users who have lawyers who make sure everything is handled well.
Comment #22
nod_Actually I thought about it and wanted to take the same approach as the libraries modules does for versions. Provide a callback in php that'll parse the JS file to look for a license block or a hardcoded license in PHP when declaring the js lib. That's pretty much the only way I can see that working programatically.
That way we can minify the file, ask the library registry for the license block of this script and add it to the minified file.
Comment #23
RobLoachThere are some handy little tools in the JavaScript/Node world that we could use:
Now, I don't mean commit these to the Drupal git repository, I mean we provide our own package.json and grunt.js scripts that instruct these tools to build/compress all of our JavaScript/CSS for us. Once they're built, we commit the built files to Drupal core. Yes, it means that whenever we make changes, to the JavaScript/CSS, we'd have to re-compile it, but that's to be expected.
This also means using AMD and require.js as we'd need a way to reference the newly built files.
In the PHP world, the two equivalent tools would translate to Composer (package manager) and Assetic (asset handler). But JavaScript libraries use JavaScript tools, "when in Rome".
Comment #24
klonosIf making the full source .js available along with the minified version removes any legal implications, then why don't we simply ship both minified and full versions of all js libraries we use? We should set a naming convention/standard for all pairs filenames: either
*.min.js
for minified versions and*.js
for full source or we could alternatively keep full source in/src
subdirectory(ies) as suggested in previous comments here.If we did something like that, then I see this procedure for updating js files:
- test everything with full source in order to ease debugging during development
- once we're ready to commit changes, minify the js files that were updated making sure any license comment blocks are left intact for legal reasons.
- make the final commit with both updated files for each library (minified and full source) as a pair
The only implication I see in this approach is that we'd have to update two files each time we make changes to a single library, but that wouldn't be much of an issue since most vendors provide both minified and full versions.
Perhaps shipping with both full and minified versions could also make it possible to provide the means so that site admins could easily switch between dev and production state (with a config setting similar to the offline mode). Production state would use the
*.min.js
files while dev would switch to using the*.js
ones (or the ones under the/src
dir if we chose to go that way).PS: If I'm off-topic here, then which would be the most appropriate issue to discuss this? File a new one perhaps?
Comment #25
RobLoachHaving both seems appropriate. jQuery and jQuery UI use npm and grunt for building/compressing themselves, which gives us
jquery/dist
andjquery-ui/dist
folders with the .min files. So, that seems like a thing we could do prior to releases. Maybe have a "min" entry in hook_library?[EDIT] @droplet mentioned...... run "grunt release" to replace @VERSION tags :-) .
Comment #26
RobLoachThis patch....
Comment #28
RobLoachHmm?
Comment #29
RobLoachIt might be i18n.
Comment #31
sunI really like the idea of having both the original + minified files readily at hand, and making Drupal use the desired variant.
The approach of switching between variants in these patches, however, looks "improvable" to me ;) I also think we should perform the update/addition of libraries with multiple variants in a separate issue + commit.
Comment #32
RobLoachThere is only a .js and a .min.js for these. It uses the 'data' tag to switch it out. Do you have a better idea on how to switch it out? Maybe add the "data" tag in
system_library_info_alter()
?Comment #33
nod_It's not directly related but any chances we get to have a core/js/jquery/{src,min} (or replace "min" with "dist") as well as mymodule/js/{src,min} a bit like psr-0 for JS :p
Comment #34
RobLoach@nod_: http://requirejs.org/docs/api.html ? :-)
Comment #35
RobLoachRe-purposing the title.
Comment #35.0
RobLoachAdded a list of libraries to update for when we add more.
Comment #36
Wim LeersLogical POV:
Since Drupal doesn't have the concept of a "build process" built-in, I'd rather not see Drupal ship with both original and minified JS files. After all, it requires humans to do a manual build process. Further: it implies that this is a best practice that contrib should do as well.
We'd essentially be bundling the same stuff twice with each download. It makes more sense to fix JS minification in core.
Practical POV:
Mostly due to legal reasons, JS minification is a very hard problem to solve. So we should go with the most practical solution for now.
HOWEVER, an interesting comment was made at #119441-149: Compress JS aggregation back in March, but nobody seems to have noticed it. In essence: a proposal by GNU to enable minification without violating licenses: http://www.gnu.org/licenses/javascript-labels.html. Maybe that makes JS minification in core viable again? :)
Comment #37
RobLoachThat's actually why we want to ship both. If there was a Drupal build process, then we could use it, along with npm/grunt to build these files.
Why not... Contrib can do it, or not do it. It's up to them. There's no way we could bring CDN module into Drupal Core. We need projects like that to exist in the contrib space so that they can accomplish all that they do.
The .min files have their licence information provided with it, and the JavaScript projects actually recommend using these files. Building with npm/grunt leaves those doc blocks in.
Comment #38
Wim LeersMy "Practical POV" line was meant to mean "it may not be ideal, but the solution proposed here is the best one we've got, so let's run with it!".
So #36 was actually a vote to support the direction taken in #24–#35 :) Sorry for not making that more clear!
The one question I still had/have is whether the JavaScript license web labels thing could be helpful. You've answered that over at #119441-152: Compress JS aggregation.
Hence: sorry for the interruption! To make up for it, I reviewed the patch in #32.
Can't we iterate over $libraries and alter the path of each file in a consistent manner? That's more understandable than this wall of text and guarantees that our directory structures are and remain consistent.
Comment #39
RobLoachAh, sorry about the confusion :-) .
Great idea! This looks much nicer. I'd rather not do string alter operations though, let's keep it fast.
Comment #40
droplet CreditAttribution: droplet commentedSeems like this is the only reason to include source code.
In case if Drupal package grows up too big. It can suggest to use Source Map for debugging. Packaged all these stuff as a developer package.
Developer package:
- jQuery Soruce, 3rd party script source
- #1532612: Move Examples Project into Drupal core
- API Docs
...
(sorry, slightly off-topic)
Comment #41
RobLoachComment #42
RobLoach#41: 1341792-41.patch queued for re-testing.
Comment #43
RobLoachDoesn't have to be done right before the release. Can be done before then.
Comment #44
tstoecklerWell, the thing is, we *definitely* want to do this before release. So if we remove the tag, this should be marked critical. I would be fine with that, but for now, re-adding tag.
Comment #45
danillonunes CreditAttribution: danillonunes commentedI'd like to see the unminified version of jQuery (and jQuery UI, etc) even in the final release. This will help a lot to developers who do heavy-js sites on Drupal. Even if we don't compress JS on-the-fly, we can ship both versions and put an option on Performance page to use full or minified versions of libraries.
Comment #46
nod_we might want to come up with a more standard way of dealing with minified files, maybe brainstorm that in #1663622: Change directory structure for JavaScript files ?
Comment #47
patcon CreditAttribution: patcon commented@droplet just tested out that source mapping thing and it's really spiffy. The potential for coffeescript or, god forbid, php-snow *shudder* is pretty interesting too :)
While it would definitely allow people to do much of the everyday debugging using the minified JS, it would still leave the old browsers high and dry if we didn't ship with uncompressed files.
And +1 for considering package managers for js libs as well!
Comment #48
RobLoach#41: 1341792-41.patch queued for re-testing.
Comment #49
mfer CreditAttribution: mfer commentedThere is an issue to add a development/production toggle to Drupal intended to specify if production or dev JavaScript should be used. This could be used for other things as well. #1537198: Add a Production/Development Toggle To Core
Ideally all of the core JavaScript would be minified and you could choose to send the dev or production versions. Contrib could even loop into this.
The Speedy Module already has scripts to do the minification and the toggling. Though, it might be better to use a tool like grunt than drush.
Comment #50
nod_We're not shipping yet :)
Comment #50.0
nod_Updated issue summary.
Comment #51
catchBumping this. Needs to be done in an early beta if not before.
Shipping both an allowing a switch is fine, but let's mark this duplicate if and when that happens.
Comment #52
catchComment #53
moshe weitzman CreditAttribution: moshe weitzman commentedWe got [ #2276219] and #2258313: Add license information to aggregated assets is nearly in. We are getting closer to minification in core which would make this issue moot. Please help!
Comment #54
nod_Not necessarily moot. UglifyJS will be better than any PHP-based minifier we'll find so having minified files still makes sense.
Comment #55
catchComment #56
xjmComment #57
catchPostponing on #2203431: [meta] Various asset (JavaScript) libraries have to be updated to a (minified) stable release prior to 8.0.0 since we could do this at the same time.
Comment #58
droplet CreditAttribution: droplet commentedcan anyone update the summary ?
Ship "both minified and un-minified versions" of all 3rd party scripts
Or
Both minified and un-minified versions of jQuery & jQuery UI only, and minified version of all other 3rd party scripts.
Or
minified versions of all 3rd party scripts
Comment #59
nod_I would go with minified version of all 3rd party scripts. I hope that will discourage people from hacking into them instead of replacing them properly. I don't see a use case for unminified in a stable core version.
Comment #60
catchComment #61
catchComment #62
Crell CreditAttribution: Crell commentednod_ The use case for including the unminified versions is debugging and development. If I'm writing code against one of those 3rd party libraries and something wonky is happening, I want to be able to step through the code in my debugger and see where it's going nutty. If the entirety of those libraries is one long line then as soon as I hit a line of code in that library my debugging is no longer useful, which sucks. :-)
That said, I could see the argument for devel providing the unminimized versions and having a toggle to enable them, assuming we make it really really easy for devel to do so. (Which would be a good smoke test to make sure the JS API is as clean as we want it to be.)
Comment #63
catchYes either devel could do that or we could add unminified versions and a toggle in a minor release, but no reason for that to block 8.0.0.
Comment #64
achtonWon't source maps help with that situation? See comment from droplet in #40.
Comment #65
webchickThis issue was marked as triaged back in November, but without a comment explaining why. IMO, this issue is a direct duplicate of #2203431: [meta] Various asset (JavaScript) libraries have to be updated to a (minified) stable release prior to 8.0.0, and we only need to amend the issue summary of that one to say "while you're upgrading external libraries, move to the minified versions." This has already been happening on all the library update patches I've seen lately.
It makes sense that we can't ship D8 with bloated source files being served by default, so I agree with a "minify them all" approach. The ability to toggle between minified and unminified (also suggested in the issue summary) is not something we need to block shipping D8 on. That sounds like a nice-to-have feature, and probably for Devel rather than core.
Comment #66
catchI don't think it's a direct duplicate - if we have any non-out-of-date libraries they'll still need swapping to the minified versions but won't be caught by #2203431: [meta] Various asset (JavaScript) libraries have to be updated to a (minified) stable release prior to 8.0.0.
Comment #67
webchickAh, yep that makes sense. Thanks!
Comment #68
cilefen CreditAttribution: cilefen commentedAs mentioned in #1341792-64: [meta] Ship minified versions of external JavaScript libraries and #1341792-40: [meta] Ship minified versions of external JavaScript libraries, source maps are the answer here. With them, you get to use a minified files, but still see un-minified when you debug. This is a little more work. For example, some 3rd party libraries may not provide maps and we would have to create them. But, it is the way things are going and it eliminates the need for a system-level switch to un-minified for debugging.
I agree with @webchick's comment in #65. This is a duplicate of #2203431: [meta] Various asset (JavaScript) libraries have to be updated to a (minified) stable release prior to 8.0.0 if we modify the requirements of that issue.
Comment #69
cilefen CreditAttribution: cilefen commented@catch I am not necessarily disagreeing with you, but why don't we insist as part of the other issue that we provide maps or a library isn't "done"? If we have two issues like this we will be stepping on our own toes.
Comment #70
cilefen CreditAttribution: cilefen commentedSource maps are supported in IE 8.1+, Firefox, and Chrome and (I can vouch for Firefox and Chrome) the debugging tools look for them by default. One way to create them:
uglifyjs foo.js -o foo.min.js --source-map foo.min.js.map
Comment #71
webchickThe part of catch's point that I think justifies the two issues is even for old libraries that we *don't* end up updating to the latest versions (because there aren't any new versions since we put them into core), they still need to be minified. However, we could try and do this in one go, and close this out quickly, then work on the other one to only handle the libraries that need updates.
That map stuff looks very interesting; had no idea that was possible.
Comment #72
hass CreditAttribution: hass commentedI do understand that these maps are useful for development, but please do not forget the file size. We may add 500kb to a page! Under normal conditions you do not need the maps at all.
I have opened a case to remove the references from js/css files when aggregated. I still think the files shouldn't be there for uncompresses files. We better only add the map files if devel is installed as one example.
Comment #73
cilefen CreditAttribution: cilefen commented@hass I for sure could be wrong about this, but as far as I know the map files are only asked-for by browser dev consoles, not at all in a normal page load.
Comment #74
hass CreditAttribution: hass commentedChrome has this enabled by default... It is downloaded all times. See the network tab, please.
Comment #75
cilefen CreditAttribution: cilefen commented@hass I know what you mean, but I don't know if that means the browser actually downloads the map files when the web console is not running.
Comment #76
cilefen CreditAttribution: cilefen commented@hass
I performed a little experiment to be sure. All the files are here.
Here are logs from my web server in each case.
Without dev tools
With dev tools open
So it looks like I am correct. The full file and the map file are downloaded only when the dev tools are open. Also, this is the point of the map files - you get to use minified files for production but you also have the benefit of non-minified files when your browser's dev console is open.
Comment #77
cilefen CreditAttribution: cilefen commentedMap files also work for CSS by the way.
Comment #78
droplet CreditAttribution: droplet commented@cilefen is CORRECT. I told the same in other threads that requested to remove sourcemap too.
Comment #79
cilefen CreditAttribution: cilefen commentedComment #80
Aki Tendo CreditAttribution: Aki Tendo commentedMinified js files can be shipped with a sourcemap, and I **strongly** advise making those maps available if minified.js is in use. The same is true for less/sass files and css.
Wrong. Very, very wrong. Step back and think for a moment - the GPL originated with Richard Stallman as a means of sharing programs written in C.
Remember, compiled code is a bit of a bear to reverse engineer back to source, and has a much stronger obfuscating nature than anything that can be done in Java Script. Therefore the requirement of the GPL has always been that you provide the source code as part of your distribution upon request. You aren't required to include it in every distribution - look at any of the major Linux distros - they rarely have the source code in the main packages since doing so trebles or quadruples the download size. But that code is available on request.
You quite obviously aren't required to ask the computer to work from that source code package. If you were then the GPL would prevent distributions of the code to anyone who didn't know how to work a compiler. Java Script and PHP's ability to work directly from the source is hardly the rule among programming languages.
Comment #81
Crell CreditAttribution: Crell at Palantir.net commentedAki: You are correct that, strictly speaking, GPLv2 only requires that the source be available upon request. Shipped in the same code download but not normally sent to the browser is fine. I said the same thing back in #6. :-) The catch is that we *do* need to retain the copyright statement, and can't strip that out as we do other comments.
I'd not heard of sourcemaps before. I'm not sure how new they are. If they work I've no problem with them.
(Disclaimer: I'm a member of the Drupal Licensing Working Group.)
Comment #82
Aki Tendo CreditAttribution: Aki Tendo commentedI've not got a dog in the fight so it doesn't really matter to me, but I don't understand a need to draw a distinction between minified and compiled code. The latter can't be read at all, and the former isn't meant to be read so placing a copyright statement in there feels moot or overkill to me. But I'm no lawyer and I've seen sabers rattled over less I guess there's nothing wrong with being over cautious.
Comment #83
nod_See #2258313: Add license information to aggregated assets.
Comment #84
stefan.r CreditAttribution: stefan.r commentedThe devel patch in #2471919: Devel only supports uncompression of jQuery and not of any other core JavaScript libraries implements the toggling of uncompressed libraries suggested by @Crell and @catch (#62, #63).
Comment #85
stefan.r CreditAttribution: stefan.r commentedActually now that we commit source maps along with external libraries, dealing with this in Devel shouldn't be needed anymore...
Comment #86
nod_Over time we got all our libs to used minified versions.
The only remaining lib is #2473837: Use minified jQuery once.
Comment #87
webchickAaaand that one's now in too. :)
I couldn't get ahold of catch today, but in #66 he pointed out that the reason this one was separate was because we needed to do that for old libraries too, not just ones we're updating. And that is now done for all old libraries. He also didn't seem to disagree with #71 where I stated we could just get this one out of the way and update the issue summary of the other.
So since classmap stuff is covered in #2400287: Remove all occurences of sourceMappingURL and sourceURL when JS files are aggregated, I am going to "be bold" here and mark this fixed!!
Thanks SO much for everyone's hard work on this!!!