Update default Closure option :output-wrapper true

[lynaghk]

Take it from a man who just lost an hour discovering one global minified function intermittently clobbering another, depending on JS load order: the Closure compiler should use :output-wrapper true by default.
That is, the emitted JS should be wrapped in a function so that it doesn't define vars in the global scope.
It costs a few bytes (enough to write an extra function(){}()) but may save a lot of people their sanity.

You can always override the default back to :output-wrapper false if you control the page.

[emezeske]

Is this an existing option to the ClojureScript compiler somewhere? I poked around a little bit but was unable to find it. I can definitely feel your pain...

[lynaghk]

Oh, my mistake; the ClojureScript compiler option is :output-wrapper true: https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/closure.clj#L927

[emezeske]

Ah, cool, thanks.

Thinking about this: is changing the default valued to true a backwards-compatibility problem? Couldn't this break existing code that relies on calling unmangled (e.g. simple output) function names? Or am I wrong about that?

[emezeske]

I added a note about this option's existence and its default value of false to the sample.project.clj file for now. I am totally onboard with making it the default, but that would probably need to be in a major release, with some accompanying documentation, and maybe a warning or something (unless I'm wrong about backwards compat issues).

[lynaghk]

Hmm, that's a good point about the simple optimizations crowd.

For adv optimizations, it'd only break existing code if people are relying on the arbitrary, minified function names emitted by Closure from another JS file, which is highly highly unlilkely (because it's crazy).
If people are using Closure advanced optimizations, they're already marking the fns they care about with ^:export true metadata, which will not be affected by this by this change.

We could flip the default depending on the optimizations flag, but it'd probably be simpler to wait until the next major version number since it is potentially breaking.
After all, I already got burned so we're putting it in all of our projects explicitly =)

[emezeske]

Sounds good. I think the simple approach of just enabling it by default for everyone is fine, but I think I'll put in a temporary (a release or two) warning message that will only be printed when simple optimizations are in use.

[cemerick]

@lynaghk Just to clarify, the problem only manifests if a page loads two advanced-compiled JavaScript files whose contents are not so wrapped?

Is this change one that should be effected in ClojureScript, rather than here?

[lynaghk]

@cemerick Not just adv-compiled, but could be any unwrapped JavaScript.
This is a longstanding problem with JS promoting vars to the global scope unless explicitly prevented from doing so via wrapping; most JS devs and libraries are familiar with it and wrap appropriately.
I think the only reason Closure doesn't do it by default is because they assume all JS on the page goes through it and leaving stuff unwrapped saves you 10 bytes or whatever (which you care about if you're The Google).

[cemerick]

Sure, sure.

I'm down with defaulting :output-wrapper true for advanced-mode compilation for a 1.0.0 release, and then always to true regardless of compilation level for a currently-theoretical breaking-OK 2.0.0 release. Pull req for the former welcome.