Read multiple forms from input

[dpsutton]

This updates clojurescript repls to behave the same as clojure repls when multiple forms are entered. This is especially helpful when pasting code into a repl. Currently only the first form is evaluated. This patch loops over the read forms evaluating them in turn.

cljs.user> 
(declare is-odd?)
(defn is-even? [n] (if (= n 0) true (is-odd? (dec n))))
(defn is-odd? [n] (if (= n 0) false (is-even? (dec n))))
#'cljs.user/is-odd?
#'cljs.user/is-even?
#'cljs.user/is-odd?
cljs.user> (is-even? 4)
true

Compared to the current behavior:

cljs.user> 
(declare is-odd?)
(defn is-even? [n] (if (= n 0) true (is-odd? (dec n))))
(defn is-odd? [n] (if (= n 0) false (is-even? (dec n))))
#'cljs.user/is-odd?
cljs.user> (is-even? 4)
Compile Warning   <cljs repl>   line:1  column:2

  Use of undeclared Var cljs.user/is-even?

  1  (is-even? 4)
      ^---

#object[TypeError TypeError: Cannot read property 'call' of undefined]
	 (<NO_SOURCE_FILE>)
cljs.user> 

NREPL cranks up a clojure.main/repl for each message which reads the input string until an EOF so this should match it. If anyone has thoughts about the flush and swaps involved here I'm all ears.

[codecov-io]

Codecov Report

Merging #98 into master will decrease coverage by 7.84%.
The diff coverage is 39.28%.

@@            Coverage Diff             @@
##           master      #98      +/-   ##
==========================================
- Coverage   83.58%   75.74%   -7.85%     
==========================================
  Files           2        2              
  Lines         195      202       +7     
  Branches        9       26      +17     
==========================================
- Hits          163      153      -10     
  Misses         23       23              
- Partials        9       26      +17

Impacted Files	Coverage Δ
src/cider/piggieback.clj	76.28% <39.28%> (-8.21%)	⬇️

---

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 06fcf78...fd99b9e. Read the comment docs.

[bbatsov]

I think you should also document this in the docstring of the function just so it's clear to everyone how this is supposed to behave.

[bbatsov]

This seems misaligned to me.

[bbatsov]

Nice catch!

NREPL cranks up a clojure.main/repl for each message which reads the input string until an EOF so this should match it. If anyone has thoughts about the flush and swaps involved here I'm all ears.

I hadn't noticed this at all before you mentioned it. 😄 Your changes look good to me, but if we can simplify the code that would obviously be even better.

Let's ping @bhauman to see what he thinks about this.

Btw, this should also be added to the changelog.

[dpsutton]

I see that cljs.repl in clojurescript has a repl function similar to the clojure version used in nrepl's interruptable eval. Perhaps we could wire this up to do the same thing that is there and we get this behavior "for free".

One issue that might preclude this though is that we have to watch for special forms:

(if (and (seq? form) (is-special-fn? (first form)))
  (do ((get special-fns (first form)) repl-env env form repl-options)
      nil)
  (eval-cljs repl-env env form repl-options))

So i'm not sure that we can get away from looping ourselves and watching for either load files or other special functions we need to intercept.

[bbatsov]

I'm afraid I'm not qualified to answer this one. :-) Using the same logic in both REPLs sounds appealing, but I don't know how to approach the problem with the different handling for special forms.

[dpsutton]

Readme updated. I'm using this all week at work to test it. I would appreciate someone else's eye's who has more familiarity, preferably @bhauman but i think he has time to give these days.

[bhauman]

This has definitely been in my mind as a possible next step. And this PR implements multiple forms the way that I think it should be done.

Starting a REPL for every for every interaction is exactly what we don't want to do. Starting a CLJS REPL is very slow and intense compared to starting a CLJ REPL. I put a bunch of work into piggieback to get away from doing just that so that we would get fast responses from the REPL.

We already have interruptible eval in every sense that its meaningful when you are talking to a REPL client running in Javascript. If your REPL hangs you can hit Crtl-C Ctrl-C in Emacs REPL buffer, then reload the page to restore your client and everything still connects and works.

[bhauman]

I will take some time to evaluate the PR some more :)

[dpsutton]

thanks for your thoughts @bhauman appreciate your experience in this area very much

[dpsutton]

i just realized a bug that this won't be able to handle: we read everything first without attaching any semantics, unlike the clojure version that spins up a repl. It will mean that we can never handle things that affect the reader like the following:

(require '[namespace :as alias])
{::alias/x 3}

[bbatsov]

@bhauman Did you find the time to think about this PR?

I was thinking myself about that problem and another idea would be to simply split the multiple forms client-side in CIDER, although this would result in a slightly different behaviour for Clojure and ClojureScript. On the other hand - this would solve the problem outlined by @dpsutton, as we'd evaluate the related forms sequentially.

[bbatsov]

@bhauman I hope you'll manage to find a bit of time in the scope of the current round of Clojurists Together to take a look at this PR, as I'd love to wrap it up at some point.

On one hand no one has complained about the existing behaviour, but on another - it'd be nice if things behaved consistently in Clojure and ClojureScript.

i just realized a bug that this won't be able to handle: we read everything first without attaching any semantics, unlike the clojure version that spins up a repl. It will mean that we can never handle things that affect the reader like the following:

I guess we can try to apply some heuristics to determine if it's ok to bucked a few expressions together or just fall back to an expression at a time in case of errors.

[eerohele]

On one hand no one has complained about the existing behaviour, but on another - it'd be nice if things behaved consistently in Clojure and ClojureScript.

Well, let me remedy that. 🙂 I got bit by this today when diving into the wonderful world of ClojureScript REPLs. In the editor plugin I'm working on, there's a command that sends the contents of the entire editor view (in practice, often a single Clojure namespace) to the nREPL server for evaluation. It's fairly confusing and inconvenient that with ClojureScript, only the first form is evaluated.