Remove Closure Compiler aggressive variable mangling

[heff]

We're currently using Google's Closure Compiler with Advanced Optimizations to get better minification of video.js, at least in comparison to uglifyjs using all the compression options.

• CC w/ AO: 68k
• UglifyJS: 89k (21k or 30% bigger)

Part of getting to that level of minification is requiring that we whitelist any API property or function that should be accessible by name after minificaiton, using exports or externs. That in itself isn't necessarily a bad thing. It means we're deliberate about what we expose, and allows us to change major internal pieces of the player without worrying that we're breaking external code or plugins.

However as more plugins have been built on top of video.js we've had a ton of requests for exporting additional properties, many of which we just overlooked. There's also the issue of sub-classing video.js components, like videojs.Button. To allow sub-classing we can't simply export methods, which makes a copy of them. We have to extern them, which prevents them from ever being mangled. Moving to externs for all components would create a lot of extra work and would also reduce some of the closure compiler benefit. (I'm not exactly sure how much, but before we started more aggressively exporting/externing properties the uglify version was over 50% bigger.)

If we were only exposing the player API this might be a different story, but video.js is also part UI library and part utility library, and those other pieces can be just as valuable to plugin developers. Exposing those other pieces means there's going to be more things that are exported than are not.

Enough background... I'm hoping to get feedback on a few things.

1. How significant is 21k? It's 21k in a player that's delivered over a billion times a month including to regions with terrible bandwidth, so it's not nothing, but are there other ways we could/should be quantifying that?
2. Are there other things we can do to improve minification in addition to or instead of UglifyJS? Is there a way we could create a blacklist of object properties that get mangled, e.g. private vars?
3. What are other things we should be considering? We'll be exposing a lot of components and utilities for the first time, and once we do that it becomes a lot harder to change them in the future. (Expose utility functions #1232 is related)

As a half answer to #2, I did some manual minification tests to see what different approaches could do for us if possible.

• Mangling private vars - savings of about 3k
• Reduce the verboseness of class method definitions, e.g. component.prototype.method = - savings of about 5k.

[gkatsev]

Don't forget that uglifyjs has other options, too. By default it doesn't do any aggressive mangling, so, we could enable more aggressive mangling. I believe it will by default not mangle function names on prototypes or stuff that it thinks should be available publicly, so, an externs file probably won't be needed.

This is closely tied with #1013 where users would include vjs themselves and have vjs minified along with their js.

With regard to filesize, we should also consider the size after gzip. However, we should also consider the headaches and bugs and wasted hours that the advanced optimization of GCC have caused developers using videojs during development.

[heff]

Gzip is a good point. I have seen less of a difference after gzipping in the past, so I'll test that again.

Re: Uglify, I used all the additional settings in this blog post.
http://davidwalsh.name/compress-uglify
Is there anything else? Before those settings it was over 100k.

[gkatsev]

Looks reasonable. We can probably play around with it for some time to tweak it for smallest size while not requiring an externs file or something.

[heff]

It would seem like the underscore prefix for private vars is more common than the suffix. As part of this process should we switch from privateVar_ to _privateVar?

While it shouldn't be a breaking change, it would change a lot of variable names. At the same time if we think it's something we'd ever do, it'd be better to do it now rather than later.

[gkatsev]

I would be for changing to a prefix. Besides being more common, as you mention, I find it more aesthetically pleasing.

[heff]

When looking at a list of object properties, underscore-first would group private vars together, rather than next to their setter/getter function. Not sure which of those I like better, but it's a data point.

[danrossi]

Confirming this is an issue, your entire public api for buttons and components is completely mangled

vjs.zencdn.net/4.11.4/video.js

What is the work around ? Building the dev project is incredibly difficult with no documentation either. Would uglify on the dev source found in the zip be the best way ?

[mmcc]

@danrossi as the title of the issue suggests, we recognize the problem and are planning to move away from GCC in the next major version. That being said, what are you looking to do that you're having trouble with? Creating custom components?

As far as building the player, the contributing guide contains pretty detailed information on how to do everything from build to submit PRs, etc. It's not as daunting as it looks, though, it can be as simple as this:

$ npm install
$ grunt

[danrossi]

I've minified the dev version

UglifyJS video.dev.js --mangle -c >> video.dev.min.js

works comes in at 91kb.

One of the components anyway trying to add a similar component as the error display alot of those public methods are mangled internally.

 videojs.NotificationDisplay = videojs.Component.extend({
        init: function(player, options){
            videojs.Component.call(this, player, options);

            this.on(player, 'notification', this.updateContent);
        }
    });

    videojs.NotificationDisplay.prototype.createEl = function(){
        var el  = videojs.Component.prototype.createEl.call(this, 'div', {
            className: 'vjs-notification'
        });

        this.contentEl_ = videojs.createEl('div');
        el.appendChild(this.contentEl_);

        return el;
    };



    videojs.NotificationDisplay.prototype.updateContent = function(e, message) {
        this.contentEl_.innerHTML = this.player().notification;
        this.show();
        setTimeout(this.hide.bind(this), 2000);
    };

That is alot of stuff to install just to get a minified dist file ! It shouldn't require node just to do that. I should be able to just run a makefile which runs UglifyJS video.dev.js --mangle -c >> video.dev.min.js no idea.

Its still trying to install stuff after 5 minutes and stuck here

gyp http GET http://nodejs.org/dist/v0.10.26/node-v0.10.26.tar.gz
gyp http 200 http://nodejs.org/dist/v0.10.26/node-v0.10.26.tar.gz

I certainly don't need node to get a file that is all. It took me hours to figure out why something wasn't working and it led me here :D

After a long while it ask me to sudo. Was it sudo npm install ?

"Please try running this command again as root/Administrator."

[danrossi]

sudo npm install seemed to help.

I get this when running it

grunt
Loading "grunt-karma.js" tasks...ERROR

Error: Cannot find module 'zeparser'

It may not help my situation if the public methods are being mangled no worries. This might help others just use uglify to minify the dev source I guess.

[mmcc]

Ahh sorry about that, I totally misunderstood you. If all you want is the dev version, you can find it in the zip (like you said originally) or in the dist folder of the stable branch. That's strange about the grunt-karma install failure, though...Travis runs npm install with every build, so...err...computers suck.

In your example, off the top of my head all of those methods should be available in the current minified version. What specifically are you running into trouble with?

We're definitely moving away from GCC, but we use tools like Grunt and others really heavily, so the Node requirement isn't going anywhere :)

[dominic-p]

Just a quick thought on this. Have you guys compared Uglify vs Closure with only simple optimizations? In my experience, Closure still wins (although by less of a margin). But, I haven't tested Uglify in a while, so maybe they have made some improvements.

[gkatsev]

The issue with GCC is that it also requires java.

[dominic-p]

True. I don't know much about the current state of the project, but it seems to me that if you already have advanced optimizations working, it should be quite easy to switch to simple. No?

[gkatsev]

Yeah, I guess it shouldn't be hard to switch, I'd like to to get rid of the java dependency. Also, we're planning to move to a more "standard" file naming conventions (#1013), so, the main file which will be unminified will be video.js and there will also be a video.min.js. The general idea would be that people would include it in their own build and minify it themselves along with all their stuff or point to our/their CDN for the min.js file in production.

[dominic-p]

+1 for the standard naming convention. That would be great.

I can understand wanting to remove a dependency. I would just recommend comparing Closure Simple vs Uglify first to see if it's worth it to keep closure (and Java) or not.

[gkatsev]

I really don't think that the difference is really big enough to matter. Also, we'll probably go through and optimize the uglify build a bit rather than just using the defaults.

[dominic-p]

Sure thing. I just thought it would be a good idea to check them both, and see what the difference is. Back when I first checked, Closure Simple was about 80% of the size of Uglify. Thanks again for a great piece of software.

[heff]

@dominic-p if you want to help us out and run a comparison of the two on the codebase, it wouldn't hurt to know either way.

[gkatsev]

Discussion is always good :)
Also, don't forget to compare after gzipping, since it doesn't matter as much what the comparison is without that.

[dominic-p]

@heff, good idea. I probably could have saved us all some time if I did that upfront. :) I ran some very basic tests on the current video.dev.js file using online interfaces for Closure and Uglify and after gzipping the output of both was almost the same ~21.5K. So, I guess it's a non-issue.

[heff]

Great! Thanks for doing that. Now we know for sure.

[heff]

#1940 was submitted to address this issue if anyone would like to review.

[heff]

Closed by #1940.