SevereOverfl0w commented 5 years ago

This makes conditional inclusion of piggieback unnecessary as it will always load, even if ClojureScript isn't provided. This makes it easier for use in tools like Cursive or as part of other "jack-in" processes.

Fix #106

I've done some basic testing of this. I'm a little unsure about just how much I'm shimming, some of it looks private, although it's marked as public (so I've retained that).

@cfleming mentioned this would be useful for Cursive, I also would like this so I can stop removing piggieback conditionally when I start my REPL. This let's me have a single command in vim, and not think about cljs or not. Maybe the same will be true for cider?

bbatsov commented 5 years ago

Thanks for tackling this! I've been wondering about a solution for this myself, but I'm still not sure what's the best approach. Perhaps just adding some hard dep to ClojureScript might be better, as it doesn't add as much complexity. I don't know how much problems this would causes for end users with respect to deps resolution, though.

SevereOverfl0w commented 5 years ago

Pros:

No risk of dependency problems Cons:
Shims may be insufficient
Development experience is slightly harder (copy-paste empty functions around for new public functions)

Dependency issues with ClojureScript I'm aware of:

ClojureScript has some old transitive dependencies, especially from the earliest version I think we support. I believe that google-closure-unshaded has some particular dependency that causes issues (just a note I saw on slack the other day)
google-closure-unshaded has an implied pair, google-closure shaded, if you have both on your classpath you will get a non-deterministic choice of which. You will possibly also get conflicting versions too!

Basically, it's dependencies can cause issues in some projects, and they're those nasty classpath conflict issues that bewilder and upset people.

bbatsov commented 5 years ago

That's a lot of deps indeed https://github.com/clojure/clojurescript/blob/master/deps.edn I had no idea... Now I've started to wonder if adding ClojureScript support to Orchard was a good idea. :D

SevereOverfl0w commented 5 years ago

org.clojure/data.json 0.2.6
org.clojure/clojure 1.9.0
org.clojure/core.specs.alpha 0.1.24
org.clojure/spec.alpha 0.1.143
org.clojure/google-closure-library 0.0-20170809-b9c14c6b
  org.clojure/google-closure-library-third-party 0.0-20170809-b9c14c6b
org.mozilla/rhino 1.7R5
com.cognitect/transit-clj 0.8.309
  com.cognitect/transit-java 0.8.332
    commons-codec/commons-codec 1.10
    com.fasterxml.jackson.core/jackson-core 2.8.7
    org.msgpack/msgpack 0.6.12
      com.googlecode.json-simple/json-simple 1.1.1
      org.javassist/javassist 3.18.1-GA
org.clojure/tools.reader 1.3.2
com.google.javascript/closure-compiler-unshaded v20180805
  com.google.jsinterop/jsinterop-annotations 1.0.0
  com.google.javascript/closure-compiler-externs v20180805
  com.google.guava/guava 25.1-jre
    com.google.errorprone/error_prone_annotations 2.1.3
    org.codehaus.mojo/animal-sniffer-annotations 1.14
    com.google.j2objc/j2objc-annotations 1.1
    org.checkerframework/checker-qual 2.0.0
    com.google.code.findbugs/jsr305 3.0.2
  args4j/args4j 2.0.26
  com.google.protobuf/protobuf-java 3.0.2
  com.google.code.gson/gson 2.7
org.clojure/test.check 0.10.0-alpha3

Here's the full tree produced by clj -Stree. I believe that the issue that I saw on slack was related to some overlap with protobuf, which also depends on Guava. Given the prevalence of guava in the Java world, having the wrong version can easily result in errors.

codecov-io commented 5 years ago

Codecov Report

Merging #107 into master will decrease coverage by 2.45%. The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master     #107      +/-   ##
==========================================
- Coverage   85.78%   83.33%   -2.46%     
==========================================
  Files           1        1              
  Lines         190       12     -178     
  Branches       11        1      -10     
==========================================
- Hits          163       10     -153     
+ Misses         18        1      -17     
+ Partials        9        1       -8

Impacted Files	Coverage Δ
src/cider/piggieback.clj	`83.33% <71.42%> (-2.46%)`	:arrow_down:

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...cf0a593. Read the comment docs.

bbatsov commented 5 years ago

@bhauman @cichli @dpsutton Any thoughts on this?

dpsutton commented 5 years ago

I'm just getting up to speed. Can someone explain the downsides of just taking a dependency on clojure.tools.reader?

SevereOverfl0w commented 5 years ago

That wouldn't be enough, that's just the first namespace that piggieback tries to require but can't.

dpsutton commented 5 years ago

I think constitutionally I'm kinda against (in-ns ...)

My thought was something along these lines my branch

(ns cider.piggieback
  "nREPL middleware enabling the transparent use of a ClojureScript REPL with nREPL tooling."
  {:author "Chas Emerick"}
  (:refer-clojure :exclude (load-file))
  (:require
   [nrepl.middleware :as middleware :refer [set-descriptor!]]))

(def ^:private cljs-present?
  (try (require 'cljs.repl)
       (require 'cider.piggieback.impl)
       true
       (catch java.io.FileNotFoundException e
         false)))

(defn cljs-repl
  "Starts a ClojureScript REPL over top an nREPL session.  Accepts
   all options usually accepted by e.g. cljs.repl/repl."
  [repl-env & {:as options}]
  (if cljs-present?
    (#'cider.piggieback.impl/cljs-repl repl-env options)
    (throw (ex-info "Clojurescript not present" {}))))

(defn wrap-cljs-repl [handler]
  (if cljs-present?
    (#'cider.piggieback.impl/cljs-aware-handler handler)
    (fn [msg] (handler msg))))

(set-descriptor! #'wrap-cljs-repl
                 {:requires #{"clone"}
                  ;; piggieback unconditionally hijacks eval and load-file
                  :expects #{"eval" "load-file"}
                  :handles {}})

and then the implementation is moved almost wholesale and no need for hijinks with namespaces.

bbatsov commented 5 years ago

That looks cleaner to me. Probably we'll need a more informative error message, though.

dpsutton commented 5 years ago

the above experiment broke completion because there was no longer a compiler environment at cider.piggieback/*cljs-compiler-env* but I see this fantastic note:

;; there's a plan to rename the main namespace of
;; piggieback to piggieback.core and the following code
;; simply paves the way for this
(def cider-piggieback?
  (try (require 'cider.piggieback) true
       (catch Throwable _ false)))

(def nrepl-piggieback?
  (try (require 'piggieback.core) true
       (catch Throwable _ false)))

so my branch now moves things into the two new files piggieback.core where the guts that depend on cljs and tools.reader live, and piggieback.api where two functions live: wrap-cljs-repl and cljs-repl.

there's only one issue left to solve that i dont understand. If you jack in without wrap-cljs-repl it seems like you can still do (cider.piggieback/cljs-repl (cljs.repl.node/repl-env)) and it will work. This does not yet work with this version. I'm not sure where it gets hooked up automatically but i'll find out.

IE, if you /usr/local/bin/clojure -Sdeps '{:deps {nrepl {:mvn/version "0.6.0"} cider/cider-nrepl {:mvn/version "0.22.0-beta6"}}}' -m nrepl.cmdline --middleware '["cider.nrepl/cider-middleware" "piggieback.api/wrap-cljs-repl"]' everything works fine with (require 'piggieback.api 'cljs.repl.node)(piggieback.api/cljs-repl (cljs.repl.node/repl-env))

but if you don't include the middleware you get a warning about being unable to set the bindings for the compiler. I don't understand why 0.4.1 works without the wrap-cljs-repl though.

cider/piggieback {:git/url "https://github.com/dpsutton/piggieback.git"
                    :sha "b830f6d7e2f28a5b8d12c7fc790bcf29b4bf84f3"}
  #_{:mvn/version "0.4.1"}

bbatsov commented 5 years ago

@dpsutton Seems the magic is here:


(run-cljs-repl ieval/*msg*
                     ;; this is needed to respect :repl-requires
                     (if-let [requires (not-empty (:repl-requires opts))]
                       (pr-str (cons 'ns `(cljs.user (:require ~@requires))))
                       (nrepl/code (ns cljs.user
                                     (:require [cljs.repl :refer-macros [source doc find-doc
                                                                         apropos dir pst]]))))
                     repl-env nil options)```

This REPL manually invokes the interactive evaluation of nREPL.

SevereOverfl0w commented 5 years ago

I think we should avoid renaming the namespace, that would be another breaking change and we only just convinced everybody to switch!

bbatsov commented 5 years ago

My idea for the renaming was to keep the old namespaces as well and do this gradually over the course of a long period of time - first updating libraries that use piggieback to check for the new namespace, wait 1-2 years, drop the compat shims then (if ever). cider-nrepl already does this and for figwheel and shadow I just planned to rename the checks they already have for cemerick.piggieback to piggieback.core. Never had the time for this and it's not that important, so it will happen when it happens (if at all).

I totally agree that we should avoid any breaking changes, and the rename would happen only after careful planning. Having consistency in the nREPL projects is important, but stability is even more important.

bbatsov commented 5 years ago

@dpsutton Any progress with your implementation? It'd be nice if we decided on the way to go forward soon.

bbatsov commented 5 years ago

(and maybe apply whatever the solution is to Orchard as well - https://github.com/clojure-emacs/orchard/issues/65)

dpsutton commented 5 years ago

I meant to comment on this last week. I think we should use @SevereOverfl0w 's implementation here. There are some subtleties that aren't quite working and I don't know why. Also my way involves some backend changes to cider-nrepl

I propose we accept the changes here and then make some patches to cider-nrepl to pave the way for a seamless transition to an piggieback.api piggieback.impl style implementation.

bbatsov commented 5 years ago

Understood.

I guess we just need to decide whether to go the shim route or the explicit dependency route. I'm still not sure that the added complexity beats the risk of dependency issues (especially given the fact that currently orchard has a hard dep on ClojureScript). I guess both of them should use shims or none of them. What do you think @SevereOverfl0w?

SevereOverfl0w commented 5 years ago

I don't think this method is particularly complex, it's certainly unconventional. But pragmatically you can figure out what's going on reasonably easy and realize that you can largely ignore the shim. This technique was taken from the Clojure source code, which has this same problem with JVM versions.

I'm opposed to the inclusion of an explicit ClojureScript dependency because there has historically been issues with conflicts due to the dependency tree of ClojureScript.

bbatsov commented 5 years ago

Okay, let's go with the shim approach. However, we'll really need to do something similar for Orchard. Guess now we need a volunteer for clojure-emacs/orchard#65. 😉

bbatsov commented 5 years ago

@SevereOverfl0w You'll need to rebase on top of the current master. It would also be nice to add a bit of explanations in the code/readme about this, so that casual readers of the code would quickly get it.

SevereOverfl0w commented 5 years ago

Adding to my list

SevereOverfl0w commented 5 years ago

diff --git a/src/cider/piggieback_impl.clj b/src/cider/piggieback_impl.clj
index 890699b..321bbe3 100644
--- a/src/cider/piggieback_impl.clj
+++ b/src/cider/piggieback_impl.clj
@@ -1,20 +1,21 @@
 (in-ns 'cider.piggieback)

 (require
- '[clojure.string :as string]
  '[clojure.java.io :as io]
  '[clojure.main]
- '[nrepl.middleware.interruptible-eval :as ieval]
- '[nrepl.misc :as misc :refer [response-for]]
- '[nrepl.transport :as transport]
- '[nrepl.core :as nrepl]
+ '[clojure.string :as string]
  '[clojure.tools.reader :as reader]
  '[clojure.tools.reader.reader-types :as readers]
  '[cljs.closure]
  '[cljs.repl]
  '[cljs.env :as env]
  '[cljs.analyzer :as ana]
- '[cljs.tagged-literals :as tags])
+ '[cljs.tagged-literals :as tags]
+ '[nrepl.core :as nrepl]
+ '[nrepl.middleware :as middleware]
+ '[nrepl.middleware.interruptible-eval :as ieval]
+ '[nrepl.misc :as misc :refer [response-for]]
+ '[nrepl.transport :as transport])

 (import
  '(java.io StringReader Writer))
@@ -289,9 +290,8 @@
   (evaluate (assoc msg :code (format "(load-file %s)" (pr-str file-path)))))

 (defn wrap-cljs-repl [handler]
-  (fn [{:keys [id session transport op] :as msg}]
-    (let [{:keys [exec]} (meta session)
-          handler (or (when-let [f (and (@session #'*cljs-repl-env*)
+  (fn [{:keys [session op] :as msg}]
+    (let [handler (or (when-let [f (and (@session #'*cljs-repl-env*)
                                         ({"eval" #'evaluate "load-file" #'load-file} op))]
                         (fn [msg]
                           (enqueue msg #(f msg))))

diff post-rebase

SevereOverfl0w commented 5 years ago

I've force pushed a new commit, but I haven't tested this one via using it. Only run lein test.

bbatsov commented 5 years ago

Thanks! I'll push a new snapshot build now.

bbatsov commented 5 years ago

The snapshot is out.

SevereOverfl0w commented 5 years ago

Nothing bad or suspicious has happened while developing Clojure code, so that's good. Haven't tested ClojureScript much in my work yet.

bbatsov commented 5 years ago

Well, I've shipped this with CIDER a week ago and no one complained, so I guess we're good. 😆

nrepl / piggieback

Shim piggieback when ClojureScript isn't loaded #107

Codecov Report