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

Fix compatibility with new Clojurescript releases (>0.0.2755) #44

Merged
merged 3 commits into from
Feb 10, 2015

Conversation

the-kenny
Copy link
Contributor

The last few Clojurescript releases broke weasel when requiring namespaces. This patch backports necessary changes from cljs.browser.repl to weasel.

1992a25 gathers upstream (js) dependencies and passes them down to the compiler. Same approach as clojure/clojurescript@5a353c4#diff-0e96f4c891374eb4ed736a5bdbb1b07dR273

74aa18e monkey-patches goog.require to be a no-op, like clojure/clojurescript@5a353c4#diff-c706571c79101d1f03c9f7073570741eR112

This fixes #43 - but depends on a clojurescript-release containing http://dev.clojure.org/jira/browse/CLJS-1002 (which isn't fixed yet). This is necessary because weasel runs cljs.closure/get-upstream-deps from a non-main thread & the function uses the current Thread's classloader. The JIRA-ticket makes get-upstream-deps take an optional argument which is then used as the classloader.

opts (assoc opts
:ups-libs (:libs ups-deps)
:ups-foreign-libs (:foreign-libs ups-deps))
compiler-env (env/default-compiler-env opts)

Choose a reason for hiding this comment

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

@the-kenny not sure if plain assoc is the right thing here. Potentially users might want to influence what's in behind these keywords?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right - I haven't thought of this. Fix incoming.

@martinklepsch
Copy link

Looking good! ✋

@martinklepsch
Copy link

After merging this we can probably close: #43, #42, #37(?), #35.
I assume many of the other existing issues might also require new verification that they still exist.

@the-kenny
Copy link
Contributor Author

#37 only partially - in in-ns stuff with cider is still broken. We should open another ticket for that.

@tvjg
Copy link

tvjg commented Feb 4, 2015

@the-kenny Actually, if you add:

[org.clojure/tools.nrepl "0.2.7"]

to your project.clj, the in-ns problem should be resolved now. See the last post on #26.

@the-kenny
Copy link
Contributor Author

The necessary fix for Clojurescript is now in master - we can merge this with the next Clojurescript-release (we just need to bump the version in :dependencies)

@tomjakubowski
Copy link
Collaborator

Thanks for this! I'll review in a few hours and if it all looks good I'll merge with the next ClojureScript release.

@tomjakubowski
Copy link
Collaborator

Looks great to me! Works splendidly with Clojurescript master. Once they cut a release I'll merge this and release with the appropriate dependencies bumped.

@bonkydog
Copy link

bonkydog commented Feb 7, 2015

I found that passing the system classloader to get-upstream-deps fixes this: #45

@the-kenny
Copy link
Contributor Author

@bonkydog That's exactly what this pull request does. We're just waiting for a new Clojurescript release. I implemented both at the same time.

tomjakubowski added a commit that referenced this pull request Feb 10, 2015
Fix compatibility with new Clojurescript releases (>0.0.2755)
@tomjakubowski tomjakubowski merged commit 5cd7009 into nrepl:master Feb 10, 2015
@tomjakubowski
Copy link
Collaborator

Thanks again for this! I've put a 0.6.0-SNAPSHOT out and I'll cut a proper release shortly.

@bonkydog
Copy link

Sweet! Thanks!

On Mon, Feb 9, 2015 at 6:09 PM, Tom Jakubowski notifications@github.com
wrote:

Thanks again for this! I've put a 0.6.0-SNAPSHOT out and I'll cut a
proper release shortly.


Reply to this email directly or view it on GitHub
#44 (comment).

bonkydog added a commit to bonkydog/stack_spike that referenced this pull request Feb 10, 2015
bonkydog added a commit to bonkydog/stack_spike that referenced this pull request Feb 10, 2015
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.

ExceptionInfo: No such namespace: cljsjs.react at line 1
5 participants