Grunt away compress_js and make Manager not broken#12611
Grunt away compress_js and make Manager not broken#12611jpdevries wants to merge 24 commits intomodxcms:masterfrom
Conversation
this commit mimicks the job of groupsConfig.php via grunt build while not reliant upon, assumes PR modxcms#11526 is merged because it packages files accordingly
this commit reduces http requests for manager js from 29 to 3 regardless of compression settings grunt build now concats and minifies the manger JS into three pairs of minified and un-minified files. keeping it broken apart in three for legacy reasons. ultimately probably should be packed into one modext/jsgrp.js file will likely submit a separate but dependent PR that packs them all in one
Keeps the same order. Even respects the js groups setting (for now)
|
I was specially asked by one of the integrators to bring some evidence to the table. This is the first of several tests I could share. I'll let the numbers speak for themselves. 2.3.5 compress_js test
compress_js enabled and compress_js_max_files set to 1000
Average: 48.12 33.44 47.30 compress_js enabled and compress_js_max_files set to 1
Average: 52.49 36.09 49.903 compress_js enabled and compress_js_max_files set to 10 (default)
Average: 50.38 | 35.86 | 44.87 compress_js disabled
Average: 47.05 | 32.36 | 46.25 |
|
This assumes MODX Revolution 2.4- with Efficiency ComparisonA) Let's say we have 350kb of scripts that when server side vsB) Everything is instead in it's own file and possibly not uglified at all ( With these user stories: A) compress_js
Cache Usage: 0% B) no compress_js
Cache Usage: 100% RememberA new URL means resending all the bytes 💯 🎤 drop |
|
Update: I removed the |
|
I don't consider that a problem really. This is is awesome and was initially what I tried to outline in #11682. I am +1 for removing the entire idea of using jsgroups. Not sure what the idea of using that ever was either. I am also +1000 everything you comment in that PR concerning removing the system settings and everything. The entire idea of serving these minified files done server side on every single request is just silly and should not be done in the first place. Nuke it and lets start fresh. |
CONFLICT (content): Merge conflict in manager/templates/default/header.tpl The login.css and index.css conflicts are common and can be ignored. The other conflicts do sound like you may need to rebase it and deal with things that changed since your first commit in June 2014. |
|
In your discussion with Jason, there was talk about leaving compress_js in to provide a dev mode that uses uncompressed/unminified core files. There's a lot of lines to scan through here so I can be overlooking that, but did you consider that in the PR? Here's what I'm seeing in the header.tpl: |
I appreciate that, and thanks for #11682! |
|
core/model/modx/modmanagercontroller.class.php line 660-667 still uses the minifier for loading CSS |
|
Those conflicts don't look too bad... I can help an integrator merge header.tpl and package.json. So this PR keeps the compress_js setting and uses it to serve a minified version of the core scripts. Extra developers would be encouraged to use this setting to determine whether or not to load minified versions of their own scripts like so https://modxcommunity.slack.com/archives/development/p1440703757000110 The current compress_js description reads:
That would need be updated. This PR never uses the core files directly. It always serves them as a single file. Anytime any of these files are touched https://github.com/jpdevries/revolution/blob/grunt-concat/_build/templates/default/gruntfile.js#L175 |
should be cleaned up now jpdevries@a25eced |
So the choice that That's not a very useful choice, and it sounds like the grunt process would actually get in the way of core developers there, and also complicates debugging should there be any issues (i.e. we can't ask someone to disable compress_js temporarily and post the file and line number to quickly figure out where a problem is). In my opinion, in the new situation, This is also what Jason asked for while you two were chatting on Slack yesterday: Jason Coward [15:56]
Looks like you actually can't? It always refers to the jsgrps(-min).js. That should be fixed so that it's possible to use the source files as it was before. Related to that, the array of files to load in The template's layout.js file is also included in the gruntfile, while this can be changed to match custom manager themes through a setting, so it should actually not be precompiled but loaded separately. Resolving the conflicts was easy enough for sure, but I do think the fact you're working on an outdated fork is a problem. It might all be working as expected for you when you test it, but when merged into the latest from git it is behaving differently and missing changes. If it's a two week old fork that's not a problem integrators can't solve, but it looks like you're more than a year behind. |
For consistency with how JS works and to make the compress_css setting description make more sense. Since 2.3 we have been serving ugly CSS no matter what…
When this is enabled, MODX will serve a compressed version of the core scripts file. When this is enabled, MODX will use a compressed version of its css stylesheets in the manager interface.
|
... making good progress though ;) |
Sourcemaps FTW You say lumping things in one big file is bad (even though that is exactly what compress_js did prior) with the exception that when it is processed with grunt you have the option of using sourcemaps. Does the current php minifier support sourcemaps? I think Jason was talking about extras not how the core files are served. I don't think anybody else really cares that all the MODXExt stuff gets dumped into one core file.
This is exactly why I was hoping you'd be willing to help out with this PR. You are a lot more familiar with this mess of code than I. |
With the current minifier, you simply disable it to work on the core source files.. |
No, that's not at all what I said. I said the choice of a big file compressed, and a big file concatted is not a very useful choice for people working on the core. |
With sourcemaps you do nothing and you get your cake and to eat it too. Here the minified script is served but as you can see the developer tools map back to the source. |
You do need to run grunt concat every time you make a change. Or set up a watch command. Either way, you're complicating the process for people to contribute to the core, which admittedly can already be fairly complex as-is with ExtJS and git. In the current situation, people that want to make a core fix can simply disable compression and don't have to worry about grunt. They commit their changes, send a pull request, and the integrators run grunt if needed (which happens regularly already due to conflicts for example). In the proposed situation, people trying to get started also need to figure out there's a grunt workflow under _build/templates/default that is used for the manager theme, oh but it's also for combining the core javascript files. And remember to run it every time they made a change, or to start the watch when they get started and to kill it when they're done. That's an extra step that takes several steps to set up if you haven't used it before. What are your objections against directly loading the source files if compress_js is off? All you've said is that sourcemaps are great (I know they are). |
this commit mimicks the job of groupsConfig.php via grunt build while not reliant upon, assumes PR modxcms#11526 is merged because it packages files accordingly
this commit reduces http requests for manager js from 29 to 3 regardless of compression settings grunt build now concats and minifies the manger JS into three pairs of minified and un-minified files. keeping it broken apart in three for legacy reasons. ultimately probably should be packed into one modext/jsgrp.js file will likely submit a separate but dependent PR that packs them all in one
Keeps the same order. Even respects the js groups setting (for now)
|
@silentworks The way this minification feature was exposed to developers was through $controller->addCss/addJavascript; that public api remains unchanged. |
I had the same thought initially, which is part of why I never finished drafting this PR for 2.3 but apparently we are OK BC wise. Also, if we reverted this commit the manager/min files would still be left in place but I figured we should remove them if we can get away with it jpdevries@3aaa912 |
|
Hmm @modxcms/integration-managers-revolution / @opengeek, could you comment on what you guys think BC wise on this issue? I'd love to merge this into 2.5 but if there are concern about people potentially interacting with the classes directly maybe targeting 3.0 would be safer. |
|
Also, reverting one commit would put the minification (not manager lol) directory back and may resolve those concerns. |
This reverts commit 3aaa912.
|
I've reverted removing the |
|
Somewhat related to what has been discussed here, this article on HTTP/2 explains how serving multiple independent files would be of benefit. |
|
Merged into 2.x for 2.5.0-rc1 in 6da4ab7, thanks @jpdevries :D |
|
🎉 |
|
Thanks @Mark-H I'll have to celebrate tonight! Woohoo! 🎉 |
|
This is awesome! |
|
🎉 |
|
MODX Developers: the As discussed in #12538 (comment) and now that this is merged, I would like to remove the "Disable CSS/JS compresson" Advanced Option from the setup screen HOWEVER, I realized this still needs to stay in 2.x because there could be edge cases where plugins are still using the I've submitted a separate PR which targets 3.x and removes the option from the installer #12857 Idea being that 3.x won't have the |
|
I agree, the min directory has to stay in 2.x to avoid BC. |
|
@OptimusCrime since #12778 will certainly lighten up the core. It actually removes more lines than this PR adds. I'm sitting in the "HTTP/2: WHAT, WHERE, WHY, AND WHEN?!" lecture at SmashingConf Oxford right now and it is refreshing to clarify, from the source, that the changes in this PR are significant performance improvements. Based on what I'm hearing, we may want to consider keeping the compress settings to default to No or even removing them completely in the anticipation of HTTP/2. Yup. The TL;DR is in a world with HTTP/2 concatenation and inlining are hacks that can be anti-optimal. UpdateIn the QA there were a few questions about concatenation and the advise was to think about caching as to how you structure (and concat) your files. So it was probably over dramatic to say no more concat at all, but it is important to effectively leverage the browser cache for performance. |
10 participants
What does it do ?
Moves the compress_css and compress_js to a front-end Grunt workflow and deletes the
manager/mindirectory.Why is it needed ?
Performance and stability improvements.
In MODX 2.4- a server-side minifier is used to concatenate and minify CSS and Javascript files that are added with the MODX API. This PR begs we abandon that methodology completely so that we can ship a more performant, more optimized, and more stable product.
Targets MODX 2.5.
I can flatten the commits if you'd likeI tried and ran away.I'm not sure why it can't automatically be merged. Probably because I started the PR a while ago. Let me know if you'd like me to rebase or reassemble it.
You won't find it in the code but this PR also asks that we have a conversation about how we look at HTTP Requests in regards to performance. They aren't all bad and in use cases like a dynamic Manager whose assets can change from page to page or as plugins are updated, activated, or deactivated to combine everything together is drastically worse for performance when measured in any practical test.
I’ve gotten a lot of resistance trying to remove these settings over the past three years however they were never tested or really architected to consider how browsers cache files so it has honestly been frustrating to be expected to prove why they need to go away considering they were never proven they should be there in the first place.
Please keep in mind that browsers cache files based off the files URL. Anyone is obviously welcome to pull from this branch and post performance and optimization findings.
What it does
<%= dirs.manager %>assets/modext/modx.jsgrps.js<%= dirs.manager %>assets/modext/modx.jsgrps-min.jscompress_settings. This means extra assets will not be grouped together. Nor should they be.removes the manager/min directory 🎉modx.jsgrps.js contains the several modxext scripts that are needed for the core. If
compress_jsis enabled themodx.jsgrps-min.jsfile will be served, but the server won't concat or minify anything at all. Extras that use the API to load CSS and JS assets will still work as expected when compress_js was disabled in 2.4-.What it doesn't do
Also see
Set compress_js and compress_css system settings to false by default. #12538 (discussion on performance and optimization)
Reduce Manager JS HTTP Requests #11527
Add notes about GZIP to Docs or Installation Instructions? modxcms/docs#94 (GZIP)
Closes
Related Issues & PRs
Note
There is a new
grunt compresstask so that thegrunt buildtask doesn't get slow from having to process all the core files each time. So rungrunt build && grunt compresswhen in doubt.Notice
Extra assets such as redactor-1.5.3.min.js are not combined with the minified core files