Fix inspector silently doing nothing if eval errored

[alexander-yakushev]

The problem
If you inspect an expression that throws an exception (e.g. (/ 1 0)), the inspector silently discards the exception as if nothing has happened. This is annoying.

Why?
Cider already has machinery for things like that, e.g. if macroexpansion throws an error, it will be displayed in a special cider middleware error buffer. That is implemented by macroexpansion middleware using with-safe-transport which, in the case of error, returns a message with status [:done :macroexpansion-error]. On the Emacs side, Cider's cider-nrepl-send-sync-request listens to :done events, and if there's an error status after it the error buffer will be raised.
Why doesn't it work for inspector? Because at some point (#164) the inspector has been changed from having its own middleware command to riding upon eval command. eval command is asynchronous and can return many results; and if the exception is thrown, it will send the exception as separate message and finish up with :done message. And cider-nrepl-send-sync-request ignores everything except :done.

What is this fix?
A hack to make eval :inspect true interceptor to short-circuit the failed eval with [:done :inspect-eval-error] message. Also, some refactoring to make the inspector middleware more like other middlewares.

Does it have to be like this?
I have no idea. I don't know if CLJS implementation will work after this, why it must use eval command, and what can be safely refactored. Need someone's help with CLJS side, this is too complicated for me.

BTW, it might be a good idea to hold this until 0.19 release.

[bbatsov]

I have no idea. I don't know if CLJS implementation will work after this, why it must use eval command, and what can be safely refactored. Need someone's help with CLJS side, this is too complicated for me.

eval is the only op we native support for ClojureScript, everything else is done in Clojure. I guess that's the reasoning behind this the breaking change, and I also guess your changes will not break this. It should be easy to test with the basic nashorn REPL or something like this.

[bbatsov]

I guess here you can explain the hack a bit, everything else looks good to me.

[bbatsov]

I guess we can give this some more meaningful name now - e.g. inspector-response.

[alexander-yakushev]

Could you explain to me or is there perhaps a place to read about this: which kinds of ClojureScript setup CIDER supports? I kinda lost it by now. Is it just the on-JVM compiler, or does it also support those self-bootstrapped things like running on Node, shadow-cljs or whatever else there is?

[bbatsov]

@alexander-yakushev http://www.cider.mx/en/latest/clojurescript/ :-)

[bbatsov]

@alexander-yakushev ping :-)

[alexander-yakushev]

Yeah, sorry, couldn't find the time to investigate the cljs portion yet.

[codecov]

Codecov Report

Merging #573 into master will decrease coverage by 0.01%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##           master     #573      +/-   ##
==========================================
- Coverage   72.84%   72.83%   -0.02%     
==========================================
  Files          36       36              
  Lines        2438     2433       -5     
  Branches      142      144       +2     
==========================================
- Hits         1776     1772       -4     
+ Misses        520      517       -3     
- Partials      142      144       +2

Impacted Files	Coverage Δ
src/cider/nrepl/middleware/inspect.clj	88.05% <88.46%> (+0.55%)	⬆️

---

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 193f267...5c596e2. Read the comment docs.

[alexander-yakushev]

Good thing I checked with ClojureScript, this patch didn't work well there:) Now I fixed that and addressed your comments.