search

umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.42k stars 2.67k forks source link

V8: Revisit javascript compression for core code #4918

Closed PeteDuncanson closed 3 years ago

PeteDuncanson commented 5 years ago

The compression we use via ClientDependancy is "good enough" up until now but javascript compression has moved on quite a bit and we should at least experiment with what we might be able to do. Along side that this could be a good place/time to discus how best to get any gains into the build process without messing up the flow too much or making it any harder to develop with.

As much as possible I'll stick to the existing build process (Gulp) and just extend that. For now I'm just focusing on core javascript and not package javascript although I'd love some tips on how package developers can compress their js in their deployed packages to get even more gains.

PeteDuncanson commented 5 years ago

I've had a play around with Terser so far (its the recommended replacement for uglify these days) https://github.com/terser-js/terser and had some good results in my tests.

There is a gulp task (https://www.npmjs.com/package/gulp-terser) to include this the build nice and easy which I'm happy to add if we think this is a goer.

Terser can do a number of things (and has many options that we can tune too) but in short in minifies, combines and analyses the code to see if it can remove any dead code or anything else to get the size down. This is way beyond what Client Dependancy currently does (or will do according to Shannon). Additionally it can "mangle" the code where it starts swapping variable names out for smaller versions. This last step could be dangerous if any of the code is being referencing outside of the bundle so should be used with caution however there are loads of options for this step too (whitelist, blacklists, various rules etc.) that we can tune to avoid any issues while still "mangling" what we can to get some size benefits.

However, for the sake of these tests I just used the default settings for mangling and I've not actually checked if this JS runs, I'm purely looking at what savings "could" be made to see if its worth while initially. Once we have some numbers then we can see if its something we want to pursue.

I went two ways with this testing.

Firstly just trying to compress umbraco.directives.js on its own. Then I also tried compressing and concatinating all the back office files that CD pulls in into one file so I could compare.

For just the directive files the logic here we could just compress each of the mini bundles that we already make as part of the existing build process (via Gulp) and then CD could still be used for combining the files on the fly as it already does but we make it skip its minification as we've already done it via another tool. Shannon discussed changing CD to "skip" files with a ".min." in their filename so this should be totally doable. So we could just add a small additional build step of just minifying the code now and then let CD concat them together. This way of working has lots of benefits as it doesn't mess with the current setup all that much so nothing new really that anyone needs to learn, the production files will just be smaller.

This would also output source maps which would get around the need for a non minified version for debugging however that might have issues with CD (https://github.com/Shazwazza/ClientDependency/issues/161). Due to those issues the second option of trying to use Terser to do the concatenating for us so the source maps "just work" and then just reference that without CD directly via a script tag or via CD if you want the cache busting etc handling for us as it does now.

Just a reminder, CD will still be used for loading in packages etc. as is.

Here is a comparison on file size from the tests I did today:

image

For just the directive file (original file size unminified is 663kb):

For the combined files (sadly I haven't got a raw bundle size to compare):

Again the mangled stuff has not been "run" to make sure it actually works so expect that to change as I'm expecting some tuning. However we do now have some numbers to go on to see if this is doable and worth while.

CD does a pretty good job out of the box and to make this worth while I think the next step is to see if that mangled option code runs as thats some big savings that are worth chasing :)

For reference this is the Terser command I was running, you might need to edit it as I was running it from within /Umbraco.Web.Ui.Client/

terser ../Umbraco.Web.Ui/umbraco/lib/jquery/jquery.min.js ../Umbraco.Web.Ui/umbraco/lib/jquery-ui/jquery-ui.min.js ../Umbraco.Web.Ui/umbraco/lib/jquery-ui-touch-punch/jquery.ui.touch-punch.min.js ../Umbraco.Web.Ui/umbraco/lib/angular/angular.js ../Umbraco.Web.Ui/umbraco/lib/underscore/underscore-min.js ../Umbraco.Web.Ui/umbraco/lib/moment/moment.min.js ../Umbraco.Web.Ui/umbraco/lib/flatpickr/flatpickr.js ../Umbraco.Web.Ui/umbraco/lib/animejs/anime.min.js ../Umbraco.Web.Ui/umbraco/lib/angular-route/angular-route.js ../Umbraco.Web.Ui/umbraco/lib/angular-cookies/angular-cookies.js ../Umbraco.Web.Ui/umbraco/lib/angular-touch/angular-touch.js ../Umbraco.Web.Ui/umbraco/lib/angular-sanitize/angular-sanitize.js ../Umbraco.Web.Ui/umbraco/lib/angular-animate/angular-animate.js ../Umbraco.Web.Ui/umbraco/lib/angular-messages/angular-messages.js ../Umbraco.Web.Ui/umbraco/lib/angular-ui-sortable/sortable.js ../Umbraco.Web.Ui/umbraco/lib/angular-dynamic-locale/tmhDynamicLocale.min.js ../Umbraco.Web.Ui/umbraco/lib/ng-file-upload/ng-file-upload.min.js ../Umbraco.Web.Ui/umbraco/lib/angular-local-storage/angular-local-storage.min.js ../Umbraco.Web.Ui/umbraco/lib/chart.js/chart.min.js ../Umbraco.Web.Ui/umbraco/lib/angular-chart.js/angular-chart.min.js ../Umbraco.Web.Ui/umbraco/lib/umbraco/Extensions.js ../Umbraco.Web.Ui/umbraco/lib/umbraco/NamespaceManager.js ../Umbraco.Web.Ui/umbraco/lib/umbraco/LegacySpeechBubble.js ../Umbraco.Web.Ui/umbraco/js/app.js ../Umbraco.Web.Ui/umbraco/js/umbraco.resources.js ../Umbraco.Web.Ui/umbraco/js/umbraco.directives.js ../Umbraco.Web.Ui/umbraco/js/umbraco.filters.js ../Umbraco.Web.Ui/umbraco/js/umbraco.services.js ../Umbraco.Web.Ui/umbraco/js/umbraco.interceptors.js ../Umbraco.Web.Ui/umbraco/js/umbraco.controllers.js ../Umbraco.Web.Ui/umbraco/js/routes.js ../Umbraco.Web.Ui/umbraco/js/init.js ../Umbraco.Web.Ui/App_Plugins/ModelsBuilder/modelsbuilder.controller.js ../Umbraco.Web.Ui/App_Plugins/ModelsBuilder/modelsbuilder.resource.js --compress --output ./js/pete_prod.js

Shazwazza commented 5 years ago

Only thing is if we start mangling things we absolutely must have source maps - which is of course part of a different discussion.

I think regardless, CDF needs to be setup so that it doesn't attempt to minify files like .min.js

PeteDuncanson commented 5 years ago

Started on the ignore min thing for CDF here https://github.com/PeteDuncanson/ClientDependency/tree/temp-ignore-min-files/ClientDependency.Core

Need to read up some more on how you reference source maps in minified files. I get it in a 1-to-1 relation but not sure about concatenating multiple files together which each have a single source map, something to research.

PeteDuncanson commented 5 years ago

Some possible helpers for stitching together the source maps include:

https://github.com/Microsoft/sourcemap-toolkit#chaining-source-maps (which has a section in the docs on exactly that)

https://github.com/benmccallum/AspNetBundling (which works with the AspNet Bundler, not sure if thats something we could use?)

Gulp v4 can generate out multiple source maps for us allowing for multiple transformations along the way. Once we switch to Gulp v4 (see #4869) then we should be able to get source maps rendered out quite nicely. Nice write up on Gupl v4 source maps here https://fettblog.eu/gulp-4-sourcemaps/ and a nice "why your source maps don't work" doc here https://scotch.io/tutorials/your-source-maps-are-broken-heres-how-to-fix-them

Then we would have to use something like the above to stitch them together in CDF...or do we? Any reason why we can't just bundle these up once and dare I say skip CDF completely? We would really only be using it for cache busting/versioning at that point and package assets too I guess?

Shazwazza commented 5 years ago

So the AspNetBundling really just uses the ajaxmin stuff underneath and ajaxmin's source maps supports is quite limited, plus we defo don't want to be shipping with all of those extra DLLs required for all of that.

And yes I agree! We should just have gulp do all of the bundling for the Core JS and CSS assets, CDF for those is then exactly like you say for cache busting. We then continue to use CDF for the dynamic loading and bundling of package files. If we do this then I think its should be reasonable 'easy' ?

PeteDuncanson commented 5 years ago

So if I play around with that sort of set up are we hopeful it will get pulled in?

I’m really conscious that I don’t want to be complicating things for the sake of it. But I think with a clean build process (ie letting gulp or whatever do all the heavy loft and be setup nice to do so) we make make most of it just “disappear”.

Any pointers off the top of your head let me know otherwise I might have a play with this one 🙂

Sent from my iPhone

On 26 Mar 2019, at 23:34, Shannon Deminick notifications@github.com<mailto:notifications@github.com> wrote:

So the AspNetBundling really just uses the ajaxmin stuff underneath and ajaxmin's source maps supports is quite limited, plus we defo don't want to be shipping with all of those extra DLLs required for all of that.

And yes I agree! We should just have gulp do all of the bundling for the Core JS and CSS assets, CDF for those is then exactly like you say for cache busting. We then continue to use CDF for the dynamic loading and bundling of package files. If we do this then I think its should be reasonable 'easy' ?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/umbraco/Umbraco-CMS/issues/4918#issuecomment-476895144, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABmNXhNaXI8p2saafC_ri1Yv_csrsVaQks5vaq5pgaJpZM4blswh.

Shazwazza commented 5 years ago

I think its a good idea, there's no reason why CDF needs to be used to combine and minify the core back office assets.

PeteDuncanson commented 5 years ago

Ok. Great to hear.

We can do this work now even before we start moving the directives around and converting to components.

If we go the route of just having one mega bundle for now (no different to what CDF) is currently doing that would be quick and easy.

Ideally we want to get to a place where we can split the bundles and then only pull in what’s needed but small steps.

I’ll play around with having a few bundles too so we can load them in parallel which should shorted the download time as well. Oooooh exciting! This is my kind of geekery 🙂

Sent from my iPhone

On 27 Mar 2019, at 00:19, Shannon Deminick notifications@github.com<mailto:notifications@github.com> wrote:

I think its a good idea, there's no reason why CDF needs to be used to combine and minify the core back office assets.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/umbraco/Umbraco-CMS/issues/4918#issuecomment-476907733, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABmNXoLN4s-egYB_9kUb44Ilt5A7ZtvCks5varkkgaJpZM4blswh.

umbrabot commented 3 years ago

Hiya @PeteDuncanson,

Just wanted to let you know that we noticed that this issue got a bit stale and might not be relevant any more.

We will close this issue for now but we're happy to open it up again if you think it's still relevant (for example: it's a feature request that's not yet implemented, or it's a bug that's not yet been fixed).

To open it this issue up again, you can write @umbrabot still relevant in a new comment as the first line. It would be super helpful for us if on the next line you could let us know why you think it's still relevant.

For example:

@umbrabot still relevant This bug can still be reproduced in version x.y.z

This will reopen the issue in the next few hours.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face: