magento / baler

AMD module bundler and preloader for Magento 2 stores.
Open Software License 3.0
175 stars 27 forks source link

Minify unbundled files with Terser #4

Open DrewML opened 5 years ago

DrewML commented 5 years ago

Requirements

  1. For each theme that is processed, all deployed .js files that are not in core-bundle.js should be minified through terser
  2. Minified files should be written in-place (same file name) to avoid the need for any path rewriting at build or runtime.
  3. Minification should happen in worker threads, to speed up compilation time and avoid blocking the main thread's bundling work

Pieces

wigman commented 4 years ago

This is what a bash script that does this looks like:

THEME_FOLDER=('pub/static/frontend/Magento/luma/')
find ${THEME_FOLDER[@]} \( -name '*.js' -not -name '*.min.js' -not -name 'core-bundle.js' -not -name 'requirejs-bundle-config.js' \) -exec terser \{} -c -m reserved=['$','jQuery','define','require','exports'] -o \{} \;
DrewML commented 4 years ago

@wigman since you work regularly with Magento, how would you expect this to work in an ideal scenario? Few options:

Destructive We replace every .js file in pub/static with its minified version. No file name changes, no backups

Additive We write minified files to {filename}.min.js, and either use a "resolver" to rewrite files (like core does), or just re-write paths in requirejs-bundle-config.js (bit harder, can hit some edge cases)

Backup We rename every .js file to {filename}.js.bak, and then write the minified version to {filename}.js

My gut wants to go with destructive, but I want to be sensitive to overwriting data because I know some folks have very long static deploy times.

wigman commented 4 years ago

Personally, I'd be fine with destructive, since that's what my command does as well. But I'm trying to think along for those that have massive amount of theme's / locales.

What about a --keep-source-files flag that does the backup ?

The question is, what's the use-case? Do you think minification can potentially fail and the command would need to run twice? In that case, how would you re-run the command so that it uses the original files again?

I haven't had a case of Terser failing before, so I'm not sure what the use-case is.

If the idea is that someone would quickly want to switch back to the "un-baled" version of the static-content. I guess a backup of the entire locale would be the quickest solution.

baler build --backup-source could first copy all the locales to en_US_source. baler build --revert-backup could remove the "baled" locales and move the originals back.

That way, people can decide for themselves if they want to have a backup for whatever reason.

Maybe we poll Twitter for some opinions?

krzksz commented 4 years ago

+1 for destructive approach, IMHO this is not baller's responsibility to handle backing up the files. In case redeploying static content is not an option for somebody, then they should make the copy themselves.

wigman commented 4 years ago

@krzksz Yes that actually makes a lot of sense.

DrewML commented 4 years ago

Sounds like we'll start with destructive 👍. Want to avoid adding config options where not critical because we'll end up like setup:static-content:deploy 😂

JelleGroenendal commented 4 years ago

Hi @wigman @DrewML , I'm failing to understand why you don't want to / cant use the default minify done by Magento. We have 7 languages / storeviews that would have to go trough terser. The total deploy then takes 1 hour, about 45 minutes of that is just running terser (server stats depended of course but it gives a bit perspective). If you use the default minify you don't have to minify separately.

TimQSO commented 4 years ago

@JelleGroenendal we run a separate .sh after the deployment on the production server itself to minify with Terser. And we minify all the storeviews with parallel processes. It just takes a couple of minutes. And because the statics are excluded from Varnish the minified .js files will be served inmediately.

After that we do a Brotli precompression for Nginx.

DrewML commented 4 years ago

I'm failing to understand why you don't want to / cant use the default minify done by Magento

Three motivators:

  1. The built-in minifier results in significantly larger bundles compared to Terser
  2. The built-in minifier doesn't actually parse the JS code, so it wouldn't be capable of advanced optimizations performed by Terser (identifier mangling, constant folding, etc)
  3. The built-in minifier does not generate source-maps for baler to ingest. It's a design goal of baler to have high-fidelity source-maps for production builds, to enable better error logging + debugging.

We have 7 languages / storeviews that would have to go trough terser. The total deploy then takes 1 hour, about 45 minutes of that is just running terser

Are you running these jobs serially? The abstraction around terser in this project spins up a collection of worker processes so everything is done in parallel.

JelleGroenendal commented 4 years ago

I'm currently running it serially. I'm using the cmd line Wigman provided

find ${THEME_FOLDER[@]} \( -name '*.js' -not -name '*.min.js' -not -name 'core-bundle.js' -not -name 'requirejs-bundle-config.js' \) -exec terser \{} -c -m reserved=['$','jQuery','define','require','exports'] -o \{} \;

I'm not sure how to spin up the workers to minify or how this part works. If they are already running then there is an issue that not all JavaScript in the static directory is minified.

TimQSO commented 4 years ago

I do this to run them parallel:

#!/usr/bin/env bash OLDIFS=$IFS IFS=$'\n' echo $datestart "Starting .js minify" for FILE in $(find /var/www/application/current/pub/static/frontend/Theme/nl_NL/ -type f -name '*.js' -not -name '*.min.js' -not -name 'core-bundle.js' -not -name 'requirejs-bundle-config.js'); do echo -n "Minifying Theme NL .js ${FILE}..." terser $FILE -c -m reserved=['$','jQuery','define','require','exports'] -o $FILE; echo "done." done & for FILE in $(find /var/www/application/current/pub/static/frontend/Theme/en_US/ -type f -name '*.js' -not -name '*.min.js' -not -name 'core-bundle.js' -not -name 'requirejs-bundle-config.js'); do echo -n "Minifying Theme EN .js ${FILE}..." terser $FILE -c -m reserved=['$','jQuery','define','require','exports'] -o $FILE; echo "done." done IFS=$OLDIFS

So in one shell script I repeat the command with & and specify all the theme folders. So there is one process per theme folder running. In our case that means 12 processes in parallel.

DrewML commented 4 years ago

If they are already running then there is an issue that not all JavaScript in the static directory is minified.

That's what this issue is open to address :)

JelleGroenendal commented 4 years ago

@GuiltyNL thanks that improved my deploy by 30 minutes :)

@DrewML missed that thanks love baler so far.

VivekShingala commented 4 years ago

While running this command, I am getting this.

zsh: no matches found: reserved=[$,jQuery,define,require,exports]

VivekShingala commented 4 years ago

While running this command, I am getting this.

zsh: no matches found: reserved=[$,jQuery,define,require,exports]

It worked fine. I had to run all the commands through bash script. I was trying it one by one on the terminal.

Thanks.