Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V8: Revisit javascript compression for core code #4918

Closed
PeteDuncanson opened this issue Mar 8, 2019 · 9 comments
Closed

V8: Revisit javascript compression for core code #4918

PeteDuncanson opened this issue Mar 8, 2019 · 9 comments
Labels
status/stale Marked as stale due to inactivity

Comments

@PeteDuncanson
Copy link
Contributor

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
Copy link
Contributor Author

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 (Shazwazza/ClientDependency#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):

  • ClientDependancy: 237 kb (64.25% reduction)
  • Terser default compression only: 209 kb (68.5% reduction, 11.8% better than CD)
  • Terser default compression and mangling: 157 kb (76.3% reduction and 33.75% better than CD)

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

  • ClientDependancy: 2,145 kb
  • Terser default compression only: 2,002 kb, 6.6% smaller than CD
  • Terser default compression and mangling: 1,579 kb, 26.4% smaller than CD

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

PeteDuncanson commented Mar 26, 2019

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
Copy link
Contributor

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
Copy link
Contributor Author

PeteDuncanson commented Mar 26, 2019 via email

@Shazwazza
Copy link
Contributor

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
Copy link
Contributor Author

PeteDuncanson commented Mar 27, 2019 via email

@umbrabot
Copy link

umbrabot commented Feb 2, 2021

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 🤖 🙂

@umbrabot umbrabot added the status/stale Marked as stale due to inactivity label Feb 2, 2021
@umbrabot umbrabot closed this as completed Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/stale Marked as stale due to inactivity
Projects
None yet
Development

No branches or pull requests

3 participants