search

nrepl / weasel

ClojureScript browser REPL using WebSockets
The Unlicense
324 stars 35 forks source link

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

Closed the-kenny closed 9 years ago

the-kenny commented 9 years ago

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 https://github.com/clojure/clojurescript/commit/5a353c418769cc6d4c212db87e5f51b9511f7c33#diff-0e96f4c891374eb4ed736a5bdbb1b07dR273

74aa18e monkey-patches goog.require to be a no-op, like https://github.com/clojure/clojurescript/commit/5a353c418769cc6d4c212db87e5f51b9511f7c33#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.

martinklepsch commented 9 years ago

Looking good! :hand:

martinklepsch commented 9 years ago

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 commented 9 years ago

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

tvjg commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

the-kenny commented 9 years ago

@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 commented 9 years ago

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

bonkydog commented 9 years ago

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 https://github.com/tomjakubowski/weasel/pull/44#issuecomment-73631520.