bhauman / lein-figwheel

Figwheel builds your ClojureScript code and hot loads it into the browser as you are coding!
Eclipse Public License 1.0
2.88k stars 210 forks source link

Cljs 1.10.x worker target support #659

Closed milt closed 6 years ago

milt commented 6 years ago

As of https://github.com/clojure/clojurescript/commit/e380903ba9af1ce9dd25691cccc1fb62c1a38d5b, ClojureScript is getting a proper compiler :target option for WebWorkers: :webworker.

This is great, as it means the hacky process I outlined here will no longer be necessary, since worker builds with :optimizations :none can now set a :main. The flipside is that figwheel needs to expect such cases during pre-compilation steps and script injection.

This pull:

I realize that this might be a little premature given how new the worker support in Cljs is, but at the very least I hope it is informative! Thanks again for figwheel!

milt commented 6 years ago

Ah, just took a look at https://github.com/bhauman/lein-figwheel/issues/638

My original solution for the localStorage issue was definitely bad.

milt commented 6 years ago

OK, figured it out! the feature? function closed over cljs.core/exists? meaning that in environments without the global object in question defined, a ReferenceError is thrown before the check runs. in #638 @boogie666 was dealing with an empty but defined object so it was sufficient, but in a worker (and probably elsewhere) localStorage is undefined, so we need to pass that through to exists? before evaluation.

As usual, the solution to a macro problem turns out to be more macros.

bhauman commented 6 years ago

This seems like it would work really well. And since it's additive there are no backwards compatability problems.

I'm assuming you have tested this and it works fine?

milt commented 6 years ago

Sorry, was out of the loop for a few days! Yeah, I've tested it in the browser, a worker, and node, don't currently have react-native set up. Thanks again!