search

taoensso / encore

Core utils library for Clojure/Script
https://www.taoensso.com/encore
Eclipse Public License 1.0
272 stars 53 forks source link

Deleted require of cljs.test and shrunk my advanced mode compile by 111KB #37

Closed petterik closed 6 years ago

petterik commented 7 years ago

Did: Was compiling my cljs project with :optimizations :advanced. Happened: Saw that cljs.pprint was included. The only library requiring it was taoensso/encore -> cljs.test -> cljs.pprint. Expected: To not have cljs.test and cljs.pprint in my advanced mode bundle.

Action: I forked encore, removed the requires of clojure.test and cljs.test and macros using them. Compiled my project with advanced mode again and bundle size went from 1 561 856 to 1 450 201 which is a 111 665 byte decrease in size. Commit: 7118d801c The macros removed was: expect and use-fixtures.

Suggestion: Moving these test macros to a new file, e.g encore_test.cljx, that only test files are supposed to include.

ptaoussanis commented 6 years ago

Hi Petter, thanks for bringing this to my attention (and for the clear report).

Would you confirm what version/s of Clojure + ClojureScript are doing this? Asking since I wouldn't have expected an otherwise unused macro to actually prevent :advanced compilation from tree-shaking out the dependency and I'm wondering whether this behaviour is still present with the latest versions of Clj + Cljs.

If so, will consider kicking out the testing utils (or at least moving them to a dedicated ns).

Cheers :-)

milt commented 6 years ago

I'm seeing this with clojure 1.9.0, clojurescript 1.9.946, happy to do any additional testing

timothypratley commented 6 years ago

This reminds me that @anmonteiro described having the same issue to me verbally at the last clojure/conj (sadly his solution was to stop using the library :( ) I completely forgot until I saw @milt comment. Just to discuss the impact of moving them; it would be a breaking change but should only affect test code which makes it feel less breaking? hehehe /shrug sounds like a good idea to me?

I'm also curious about why ClojureScript dead code elimination is unable to remove it.

anmonteiro commented 6 years ago

ClojureScript's defmulti / defmethods can't currently be dead-code eliminated, since they rely on runtime dispatch and there is no static information at compile-time about which methods will be called.

That said, both cljs.test and cljs.pprint (which cljs.test requires) both make use of defmulti for their implementation, which in turn causes this issue.

timothypratley commented 6 years ago

I see; thank you for the root cause explanation @anmonteiro :)

ptaoussanis commented 6 years ago

Likewise, thank you @anmonteiro- really appreciate the insight. That helps make the call here easy since it takes off the table the possibility of other cleaner solutions.

@timothypratley Will cut a new release in a moment, thanks for pinging about this :+1:

ptaoussanis commented 6 years ago

[com.taoensso/encore "2.94.0"] on Clojars now, should resolve this issue.