JShrink ~1.0.1 dependency munges javscript files

[apiuser]

Feature request from keithbentrup, posted on GitHub Jan 19, 2016

Steps to reproduce 1) Install the reference store with a sample data 2) Configure js minification and deploy static assets 3) Visit a product page Expected result: Product page loads with product image and no js error in the js console Actual result: Product pages with no product image and a js error

Root cause: JShrink munges lib/web/fotorama/fotorama.js. In my tests, JShrink 1.0.1 and JShrink 1.1.0 both munge the javascript when minifying.

This is a common problem with javascript minifiers. I've found yuicompressor to be much better, and when I minified with fotorama.js with yuicompressor, it worked.

[apiuser]

Comment from guz-anton, posted on GitHub Jan 25, 2016

Keith, thanks for investigation. Yes, we also found that JShrink minifier corrupt fotorama.js. So as quick solution we place fotorama.min.js nearby main file.

Your proposal about yuicompressor is good solution. But it is an Java application. Magento cann't rely on +1 technology in critical application flows. Otherwise we could use NodeJs + UglifyJs as engine for minifying.

So at this moment we are not going to use yuicompressor or Google Closure Compiler as main engine for processing assets.

[apiuser]

Comment from keithbentrup, posted on GitHub Jan 25, 2016

@guz-anton Ok, there is a secondary issue here though. The underlying filesystem affects which file is returned first for minification. On linux, the code reads fotorama.min.js first, copies the file to pub/static/, and continues without issue. In my tests on OSX, the code reads fotorama.js first, minifies (and BREAKS it!), copies the file to pub/static/, and continues to fotorama.min.js from the Magento dir. However, now since fotorama.min.js exists in the destination folder pub/static, it will not overwrite the broken file with the existing fotorama.min.js from the source folder. (Should we record this as a separate bug?)

Either way, broken minification definitely needs to be addressed. I don't think using JShrink (something known to break valid js) is an acceptable solution, and I would argue that Magento is already clearly relying on "+1" technology for many other scenarios. It doesn't have it's own built in app server or it's own built in database server.

I don't want to have to rely on Magento for it's "own" broken javascript minifier (which is built (poorly) by a third partly anyway). I would definitely consider js minification outside the scope of an ecommerce platform, and therefore the best available solution(s) should be considered, and we should not just use a bad one because it's php based.

[apiuser]

Comment from southerncomputer, posted on GitHub Jun 08, 2016

https://github.com/magento/magento2/issues/4506 this is a duplicate ?

[apiuser]

Comment from guz-anton, posted on GitHub Jul 15, 2016

Hi @keithbentrup We are going to make investigation under internal ticket MAGETWO-52632. If you have any comment or suggestion you are welcome.

@southerncomputer, no, looks like #4506 is not a duplicate.

[apiuser]

Comment from southerncomputer, posted on GitHub Jul 15, 2016

@keithbentrup are you sure this in not caching at the apache/nginx - or php realpath or php opcache accelerator that is racing it's own caching (file_exists) against dynamic creation of files?

I've found this to be the case with php_opcache - so i disabled revalidation and manually do service php-fpm restart after any code/config/cache change (plus service nginx restart plus service varnish restart) - plus i disable CLI opcache mode since that can't be easily flushed!