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

Use UglifyJS to minify files in minified target #6939

Merged
merged 1 commit into from
Feb 13, 2016

Conversation

TimothyGu
Copy link
Contributor

It is written in JavaScript, is less buggy, and compresses better.

@timvandermeij
Copy link
Contributor

I like the idea of this. In addition to what you mention, less code is required and it can be downloaded automatically as it is an NPM dependency. We do need to make sure that this uglifier does not break the final build. Did you verify that already?

@TimothyGu
Copy link
Contributor Author

@timvandermeij Yes. I've been using UglifyJS in production and no bugs have been reported or observed because of the minifier.

It is written in JavaScript, is less buggy, and compresses better.
@TimothyGu
Copy link
Contributor Author

Seems like I spoke too soon. Google Chrome chokes on minified files after 2edf279 (committed 4 days ago) because of long sequences generated by UglifyJS. It only affects pdf.worker.js, so I have added a workaround for that file specifically. The workaround increases the output size by less than 1KB, which is less than 0.2% of the total file size.

On the other hand, I did some benchmarking, and found that Closure Compiler compiles all three files marginally better (difference <1%), at the cost of 8 times more CPU time. The fact that after gzipping one of the files compiled by UglifyJS is smaller than the counterpart produced by Closure doesn't help Closure's case either.

So all in all, even though some Chrome bug makes UglifyJS's output look "buggy," its sheer performance and good-enoughness makes it a lot better choice.

@timvandermeij
Copy link
Contributor

@yurydelendik What do you think about this patch?

@yurydelendik
Copy link
Contributor

target.minified is not used on the botio. @timvandermeij if it works as advertised then we can accept it. (I saw somewhere messages about UglifyJS has some problem with our new code base, oh it's above :).

@timvandermeij
Copy link
Contributor

I did more testing with this and found no further issues. Thank you for the patch!

timvandermeij added a commit that referenced this pull request Feb 13, 2016
Use UglifyJS to minify files in minified target
@timvandermeij timvandermeij merged commit cd9d134 into mozilla:master Feb 13, 2016
@timvandermeij
Copy link
Contributor

I have just updated the frequently asked questions wiki page to mention what has been changed in this update.

@TimothyGu TimothyGu deleted the uglifyjs branch February 13, 2016 16:03
@TimothyGu
Copy link
Contributor Author

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants