:optimizations :advanced not wrapping output by default #64

corbt · 2015-03-16T12:20:53Z

Hey, I recently started a new ClojureScript project using boot-cljs. Everything worked fine, except that I noticed that when I compiled with the :optimizations :advanced mode of the closure compiler the main.js file that came out clobbered my global namespace.

I was able to find the :output-wrapper parameter of the clojurescript compiler, and turning it on in my build.boot file fixed the issue. However, my understanding is that with the advanced compilation options, this should be turned on by default.

I've included my build.boot file below, in case it helps. Very much vanilla stuff though. The only change between the old and new versions is the :compiler-options on the last line of the new one.

Old

; old build.boot
(set-env!
 :source-paths   #{"src"}

 :dependencies '[[adzerk/boot-cljs      "0.0-2814-3" :scope "test"]
                 [adzerk/boot-cljs-repl "0.1.9"      :scope "test"]
                 [adzerk/boot-reload    "0.2.6"      :scope "test"]

                 [reagent "0.5.0" :exclusions [cljsjs/react]]])

(require
 '[adzerk.boot-cljs      :refer [cljs]]
 '[adzerk.boot-cljs-repl :refer [cljs-repl start-repl]]
 '[adzerk.boot-reload    :refer [reload]])

(deftask dev
  "Watch/compile clojurescript files in development"
  []
  (comp
    (watch)
    (cljs-repl)
    (cljs :source-map true
          :optimizations :none)))

(deftask prod
  "Compile clojurescript files for production"
  []
  (comp
    (cljs :optimizations :advanced)))

New

; build.boot
(set-env!
 :source-paths   #{"src"}

 :dependencies '[[adzerk/boot-cljs      "0.0-2814-3" :scope "test"]
                 [adzerk/boot-cljs-repl "0.1.9"      :scope "test"]
                 [adzerk/boot-reload    "0.2.6"      :scope "test"]

                 [reagent "0.5.0" :exclusions [cljsjs/react]]])

(require
 '[adzerk.boot-cljs      :refer [cljs]]
 '[adzerk.boot-cljs-repl :refer [cljs-repl start-repl]]
 '[adzerk.boot-reload    :refer [reload]])

(deftask dev
  "Watch/compile clojurescript files in development"
  []
  (comp
    (watch)
    (cljs-repl)
    (cljs :source-map true
          :optimizations :none)))

(deftask prod
  "Compile clojurescript files for production"
  []
  (comp
    (cljs :optimizations :advanced
          :compiler-options {:output-wrapper :true}))) ; added the compiler options clause

The text was updated successfully, but these errors were encountered:

Deraen · 2015-05-17T19:07:19Z

This is a bug with either cljs compiler or documentation, I'll ask David.

Deraen · 2015-05-17T19:13:50Z

The docs were wrong. Default is false.

kanwei · 2015-12-23T15:41:25Z

Reopening this to say that :output-wrapper should be true by default on :advanced... emezeske/lein-cljsbuild#201

Deraen · 2015-12-23T15:46:38Z

That issue is quite old so I don't know how well it does hold now. Cljsbuild does indeed seem to use output-wrapper with advanced compilation.

kanwei · 2015-12-23T15:48:32Z

I think it's pretty important to set this on, since it's a super obscure source of bugs (globals clattering) and people switching from lein-cljsbuild (like me), which does have it on, are going to suddenly have potentially broken builds on :advanced.

Deraen · 2015-12-23T15:54:12Z

If this is so important, I think the default option should be set on ClojureScript compiler itself instead of on build tool.

This should be reported here: http://dev.clojure.org/jira/browse/CLJS (I did a quick check and did not see similar issues)

Cljs wiki used to say this is enabled when using advanced compilation, but as this is not the case, I remeved the mention:
https://github.com/clojure/clojurescript/wiki/Compiler-Options/_compare/f05a5247c827f19307e9ebce4e328378d3ab9144...abd8b21239b872e8f5fcb8773c659c5a1a2badc5

Deraen · 2016-01-09T21:39:04Z

Looks like ClojureScript default is not going to change: http://dev.clojure.org/jira/browse/CLJS-1520

Deraen · 2016-01-09T21:46:22Z

Would it be enough to create wiki pages "Differences to Lein-cljsbuild" and "Common problems" and list this there?

Would need some description of the problem and cause as I have not seen this myself.

kanwei · 2016-01-09T21:52:27Z

Yeah it would definitely be nice to have some page like that. Basically without :output-wrapper, advanced compilation creates global variables like "aE", "bC", etc (minimized) and these may interfere with existing globals on the page.

Deraen · 2016-01-09T21:54:21Z

So it requires some non-Closure/Cljs JS code to trigger this? Minified code from foreign-libs (cljsjs) can probably trigger this?

kanwei · 2016-01-09T22:00:18Z

I think foreign-libs are fine (if they go through the advanced compilation with the entire codebase). It's more of a problem when putting the compiled js on a page with other non-cljs code (Wordpress for example) and causing conflicts.

martinklepsch · 2016-01-10T00:16:55Z

I think @Deraen's description is accurate. CLJSJS code can certainly trigger these kinds of failures.

Might be worth considering adding a warning to cljs task when any deps within the CLJSJS group are used and output-wrapper is not enabled. Special casing things is bad I guess but depending on how easy it is to integrate we might just do it?

@kanwei foreign-libs don't go through advanced compilation but instead may provide their own minimized builds. Only sources properly annotated for the closure compiler can be compiled with advanced and most JavaScript libraries are not.

Deraen · 2016-01-10T20:37:39Z

https://github.com/adzerk-oss/boot-cljs/wiki/Troubleshooting
https://github.com/adzerk-oss/boot-cljs/wiki/Differences-to-Lein-cljsbuild

pesterhazy · 2016-12-13T10:56:29Z

I just ran into this issue. If you use a lot of third-party javascript libraries, you're very likely to run into weird errors because those libraries overwrite two-letter js globals. What's worse, these show up only intermittently because variable assignments change quasi-randomly for every build.

Although this behavior is documented, it's hard to know what to google for or that the place to look is the cljs compiler options.

I see three ways of fixing this:

Use output-wrapper for all optimization types. Pros: consistency; Cons: differences to cljs compiler defaults and lein-cljsbuild, breaking change from previous boot-cljs versions
Use output-wrapper=true only for advanced optimization. Pros: consistency with lein-cljsbuild; cons: magical change in js structure between advanced and whitespace optimization, breaking change from previous boot-cljs versions.
No change in behavior but issue a warning on advanced compilation

I'd be happy with each option (possibly favoring 1). If there's a consensus, I'll happily prepare a PR.

martinklepsch · 2016-12-13T11:09:32Z

I'm inclined to vote for 3 just because there's no clearly right way. Also sticking to compiler defaults as defaults makes things more predictable most of the time I guess.

We could show the warning for all optimization settings but :none as artifacts created using these modes are much more likely to be used as production builds. Also I think the warning can be multiple lines, perhaps involving some ascii art, so that people really don't miss it.

pesterhazy · 2016-12-13T11:16:16Z

I love the ascii art idea

martinklepsch · 2017-01-19T16:32:14Z

@Deraen @pesterhazy — replying to your Slack convo:

Juho said:

adding random JS files is always dangerous

I don’t think that’s something most people are aware of and thus think adding this warning is a great idea.

If you use advanced and external JS and are not aware that you should be setting output-wrapper to true you will very likely run into trouble in the future.

I also don't consider this breaking since it doesn't affect the result (i.e. the compiled JS) of the task. Setting output-wrapper to true by default would be a breaking change and after some discussion above a warning seems like the most sensible approach to avoid any breaking changes.

danielsz · 2017-06-23T20:30:23Z

I've had exactly the same issue. I've spent a lot of time to finally stumble on this ticket and solve my problem. I don't know what the correct decision should be, but something must be done to save people's minds. I was so desperate I even tweeted about it: https://twitter.com/danielszmu/status/878317585820819456

danielsz · 2017-06-23T22:14:16Z

Keeping the defaults of the build tool aligned with the defaults of the compiler is a good, general principle.
However, here are a couple of reasons why we might want to change the defaults for the particular case of output-wrapper in boot-cljs.

Omitting output-wrapper in advanced builds is an obscure source for bugs in production
lein-cljsbuild is enforcing output-wrapper in advanced builds for exactly that reason.
The reason it is not enforced in the compiler is because of fear of breakage. And not a principled motivation.
We, in turn, don't break anything for our users. We actually have their back.
David Nolen is OK with the change downstream. https://clojurians.slack.com/archives/C03S1L9DN/p1498255301728455

kanwei · 2017-06-23T22:25:16Z

@danielsz I have been arguing for it to be on by default as well, thank you for voicing your input too! It's definitely a bad experience for first-time users.

martinklepsch · 2017-06-24T11:41:33Z

@Deraen what do you think about setting :output-wrapper to true by default when using advanced compilation? I still think it's good not to mess with defaults but setting it to true is clearly the more friendly behavior.

We could document differences to default compiler options in the task docstring + Readme.

fix #64; set output-wrapper true if unset and advanced compilation

corbt changed the title ~~:optimizations :advanced not wrapping output by default~~ :optimizations :advanced not wrapping output by default Mar 16, 2015

corbt changed the title ~~:optimizations :advanced not wrapping output by default~~ :optimizations :advanced not wrapping output by default Mar 16, 2015

Deraen closed this as completed May 17, 2015

Deraen reopened this Dec 23, 2015

pesterhazy mentioned this issue Dec 21, 2016

Warn about optimizations without output-wrapper #145

Closed

danielsz mentioned this issue Jun 23, 2017

Bad package (only under advanced compilation) taoensso/sente#306

Closed

Deraen closed this as completed in a9ba781 Jul 4, 2017

Deraen added a commit that referenced this issue Jul 4, 2017

Merge pull request #164 from boot-clj/output-wrapper-true

206d599

fix #64; set output-wrapper true if unset and advanced compilation

kanwei mentioned this issue Apr 24, 2018

Add :output-wrapper option thheller/shadow-cljs#260

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

:optimizations :advanced not wrapping output by default #64

:optimizations :advanced not wrapping output by default #64

corbt commented Mar 16, 2015

Deraen commented May 17, 2015

Deraen commented May 17, 2015

kanwei commented Dec 23, 2015

Deraen commented Dec 23, 2015

kanwei commented Dec 23, 2015

Deraen commented Dec 23, 2015

Deraen commented Jan 9, 2016

Deraen commented Jan 9, 2016

kanwei commented Jan 9, 2016

Deraen commented Jan 9, 2016

kanwei commented Jan 9, 2016

martinklepsch commented Jan 10, 2016

Deraen commented Jan 10, 2016

pesterhazy commented Dec 13, 2016

martinklepsch commented Dec 13, 2016

pesterhazy commented Dec 13, 2016

martinklepsch commented Jan 19, 2017 •

edited

Loading

danielsz commented Jun 23, 2017

danielsz commented Jun 23, 2017 •

edited

Loading

kanwei commented Jun 23, 2017

martinklepsch commented Jun 24, 2017

:optimizations :advanced not wrapping output by default #64

:optimizations :advanced not wrapping output by default #64

Comments

corbt commented Mar 16, 2015

Old

New

Deraen commented May 17, 2015

Deraen commented May 17, 2015

kanwei commented Dec 23, 2015

Deraen commented Dec 23, 2015

kanwei commented Dec 23, 2015

Deraen commented Dec 23, 2015

Deraen commented Jan 9, 2016

Deraen commented Jan 9, 2016

kanwei commented Jan 9, 2016

Deraen commented Jan 9, 2016

kanwei commented Jan 9, 2016

martinklepsch commented Jan 10, 2016

Deraen commented Jan 10, 2016

pesterhazy commented Dec 13, 2016

martinklepsch commented Dec 13, 2016

pesterhazy commented Dec 13, 2016

martinklepsch commented Jan 19, 2017 • edited Loading

danielsz commented Jun 23, 2017

danielsz commented Jun 23, 2017 • edited Loading

kanwei commented Jun 23, 2017

martinklepsch commented Jun 24, 2017

martinklepsch commented Jan 19, 2017 •

edited

Loading

danielsz commented Jun 23, 2017 •

edited

Loading