Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cljs 1.10.x worker target support #659

Merged
merged 3 commits into from
Mar 5, 2018

Conversation

milt
Copy link
Contributor

@milt milt commented Mar 1, 2018

As of clojure/clojurescript@e380903, 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:

  • Adds :webworker to the valid targets in the spec
  • Shadows the node checks during injection for workers
  • Addresses an odd behavior where the feature? check during the definition of local-persistent-config throws a runtime error. Since this check is present in previous versions of figwheel that work fine with workers, and the same check runs fine inside a function, I'm guessing it has something to do with changes to the cljs compiler, so my fix for this should be regarded with suspicion.

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!

@@ -120,7 +120,8 @@

(defonce local-persistent-config
(let [a (atom {})]
(when (feature? js/localStorage "setItem")
(when (and (html-env?)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the description, I don't understand why feature? doesn't work on its own here as it has in the past. Explicit checking for html-env? seems naive, since it might be provided by a polyfill/lib in non-browser environments.

@milt
Copy link
Contributor Author

milt commented Mar 1, 2018

Ah, just took a look at #638

My original solution for the localStorage issue was definitely bad.

@milt
Copy link
Contributor Author

milt commented Mar 2, 2018

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
Copy link
Owner

bhauman commented Mar 2, 2018

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?

@bhauman bhauman merged commit db2cd28 into bhauman:master Mar 5, 2018
@milt
Copy link
Contributor Author

milt commented Mar 6, 2018

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants