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

Bump to cider/piggieback #730

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Bump to cider/piggieback #730

wants to merge 1 commit into from

Conversation

viesti
Copy link

@viesti viesti commented Mar 23, 2019

Allows use with Leiningen 2.9.1.

This would allow upgrading projects that still use lein-figwheel to work with Leiningen 2.9.1, which brings nrepl/nrepl (mentioning the organization here for clarity).

On a related note, upgrading org.clojure/tools.nrepl to nrepl/nrepl as discussed in #718 might also be in order, although I guess the newer nrepl get's bundled in when used with Leiningen 2.9.1.

Allows use with Leiningen 2.9.1
@bhauman
Copy link
Owner

bhauman commented Mar 23, 2019

The code should currently work with Lein 2.9.1. Does it not? The :dev dependency should not influence use of the plugin.

I should change the :dev dependency so that folks could can work on it. But this string replace patch doesn't take into account the context of the changes. Which already account for using cider/piggieback and also allow the use of earlier versions of piggieback and lein.

But I will check against lein 2.9.1 and verify this.

@viesti
Copy link
Author

viesti commented Mar 23, 2019

Thanks for the fast reply! And apologies for the lack of context :/

What set me on this path was encountering the following on an older project that uses cemerick/piggieback "0.2.1" and duct/figwheel-component "0.3.3" while using Leiningen 2.9.1:

[WARNING] No nREPL middleware descriptor in metadata of #'cemerick.piggieback/wrap-cljs-repl, see nrepl.middleware/set-descriptor!
nREPL server started on port 62473 on host 127.0.0.1 - nrepl://127.0.0.1:62473
ERROR: Unhandled REPL handler exception processing message {:id 4f377039-b544-4d1d-87c9-fa62756895f3, :op clone}
java.lang.NullPointerException
	at clojure.core$deref_future.invokeStatic(core.clj:2290)
	at clojure.core$deref.invokeStatic(core.clj:2310)
	at clojure.core$deref.invoke(core.clj:2296)
	at cemerick.piggieback$wrap_cljs_repl$fn__38772.invoke(piggieback.clj:288)
	at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__352.invoke(middleware.clj:22)
	at nrepl.server$handle_STAR_.invokeStatic(server.clj:18)
	at nrepl.server$handle_STAR_.invoke(server.clj:15)
	at nrepl.server$handle$fn__31739.invoke(server.clj:27)
	at clojure.core$binding_conveyor_fn$fn__6772.invoke(core.clj:2020)
	at clojure.lang.AFn.call(AFn.java:18)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

Bumping piggieback to cider/piggieback and nrepl to nrepl/nrepl helped to remove the error (this is where I ran transitively into sidecar). It might be that the com.cemerick/piggieback lineage doesn't work with current nrepl, but I didn't yet find a definite root cause. I guess eventually the most neat thing would be to move this project in question to figwheel-main, but that might require some effort :)

Anyways, thank you! I'll try narrowing the cause, and if I'm off the track, you'r welcome to reject this PR :)

@bbatsov
Copy link

bbatsov commented Apr 20, 2019

Yeah, the old piggieback doesn't work with modern releases of nREPL, but I think @bhauman was making the point that the code as currently structured allows the use of older version of Lein.

I think that probably most people have already migrated off Lein 2.8.1, so it's probably a good idea to drop the conditional checks and simplify the code. That would be a nice follow-up to 2b67fa4

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.

3 participants