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

Move from uglify to closure #4286

Closed
wants to merge 2 commits into from
Closed

Move from uglify to closure #4286

wants to merge 2 commits into from

Conversation

kristoferbaxter
Copy link

@kristoferbaxter kristoferbaxter commented Jan 10, 2020

Move from Uglify to Closure Compiler

Result

dist/instantsearch.production.min.js: 241.09 KB/63.25 KB => dist/instantsearch.production.min.js: 238.58 KB/62.34 KB

@francoischalifour
Copy link
Member

Thanks for this initiative!

I don't get the same size results for the Minified + GZipped bundle with Bundlesize:

  • Before (Uglify): 63.24 KB
  • After (Closure): 65.06 KB

This would mean that Uglify leads to a better GZipped bundle.

Is that something that you would have expected?

@Haroenv
Copy link
Contributor

Haroenv commented Jan 15, 2020

It's indeed pretty surprising. Maybe closure needs to be configured to remove more things?

@kristoferbaxter
Copy link
Author

kristoferbaxter commented Jan 16, 2020

Apologies for delay: here are some results with different minfiers.

Results

compiler + terser terser compiler uglify
62.39KB 63.64KB 64.12KB 63.24KB

Based on this I'd recommend moving to compiler + terser.

Future Opportunities

This configuration uses Closure Compiler with SIMPLE_OPTIMIZATIONS (see documentation for differences between optimization levels).

With a few tweaks I was able to build the source with ADVANCED_OPTIMIZATIONS, and the result was 56.17KB. A significant improvement.

To move to ADVANCED_OPTIMIZATIONS, you'd want to ensure that valid Closure types or externs are present for the entire source. (This is doable, but significantly more work).

For instance, right now this fails due to invalid syntax usage in hogan.js. Here's the associated PR with a fix (twitter/hogan.js#264).

if (partial.subs) {
        // Make sure we consider parent template now
        if (!partials.stackText) partials.stackText = {};
        for (key in partial.subs) {
          if (!partials.stackText[key]) {
            partials.stackText[key] = (this.activeSub !== undefined && partials.stackText[this.activeSub]) ? partials.stackText[this.activeSub] : this.text;
          }
        }
        template = createSpecializedPartial(template, partial.subs, partial.partials,
          this.stackSubs, this.stackPartials, partials.stackText);
      }

Closure would like key to be declared (const key in ...).

@francoischalifour
Copy link
Member

francoischalifour commented Feb 18, 2020

Thanks for this analysis! We decided not to move forward with this solution for now because it involves some changes in our setup (dependencies, banners in the final bundles, stripping out comments, etc.) and the overall bundle size doesn't decrease significantly.

I'm personally really excited that you made Google Closure Compiler accessible to Rollup users because it was a pain to integrate in a frontend library without this plugin.

Thank you for the PR – we'll reevaluate this tool when we plan to rework on our build script.

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

Successfully merging this pull request may close these issues.

3 participants