nrepl / piggieback

nREPL support for ClojureScript REPLs
480 stars 48 forks source link

Acknowledge cljs dependency in `tools.deps` docs #104

Closed duncanjbrown closed 5 years ago

duncanjbrown commented 5 years ago

When I followed the instructions to run an nREPL using clj I got a compilation error containing the following message:

java.io.FileNotFoundException: Could not locate clojure/tools/reader__init.class, clojure/tools/reader.clj or clojure/tools/reader.cljc on classpath.

Adding a deps.edn dependency on clojure.tools.reader fixes that, but reveals another dependency error. It eventually becomes clear that Piggieback simply depends on ClojureScript.

Add a note to the README to help the next person.

codecov-io commented 5 years ago

Codecov Report

Merging #104 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #104   +/-   ##
=======================================
  Coverage   85.78%   85.78%           
=======================================
  Files           1        1           
  Lines         190      190           
  Branches       11       11           
=======================================
  Hits          163      163           
  Misses         18       18           
  Partials        9        9

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cbc255e...4f293a1. Read the comment docs.

bbatsov commented 5 years ago

That's a good point, but I'm not sure how to best document it. Generally we operated with the assumption that ClojureScript would be provided by the user's environment and opted not to hardcode a dependency on it that sometimes might be problematic (e.g. outdated). I agree we should probably write something, but I'm not sure what exactly.

@cichli @dpsutton @bhauman Any thoughts on this?

duncanjbrown commented 5 years ago

Thank you @bbatsov. I felt a bit awkward about that too.

If users are asked to add it to their deps.edn then I guess the work of deciding which version to include is already on their plate anyway. So a possible solution is to say "Alternatively, you can pass the dependency to clj by adding an -Sdeps option to the above command"?

SevereOverfl0w commented 5 years ago

I've been wanting to raise a similar issue to this, I was going to look into making piggieback disable itself if cljs isn't available

bbatsov commented 5 years ago

This was addressed by #106.