-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
JShrink ~1.0.1 dependency munges javscript files #3040
Comments
Keith, thanks for investigation. Yes, we also found that JShrink minifier corrupt 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. |
@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. |
#4506 this is a duplicate ? |
Hi @keithbentrup @southerncomputer, no, looks like #4506 is not a duplicate. |
@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! |
Thank you for your submission. We recently made some changes to the way we process GitHub submissions to more quickly identify and respond to core code issues. Feature Requests and Improvements should now be submitted to the new Magento 2 Feature Requests and Improvements forum (see details here). We are closing this GitHub ticket and have moved your request to the new forum. |
Why is this issue closed? |
Steps to reproduce
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.
The text was updated successfully, but these errors were encountered: