Avoid setting the repl type on each and every nRepl message

[arichiardi]

This PR has been opened mainly to discuss about this behavior.

When these two lines are in, the cider-repl-type variable is always set to
what is coming to nRepl - we trust that nRepl knows if it is in a cljs rather
then a clj REPL.

This seems not to be that true, according to my tests with a shadow-cljs REPL:

<cider-connect-clojurescript>

(do (require 'shadow.cljs.devtools.api) (shadow.cljs.devtools.api/nrepl-select
:dev))

;; in ielm
<cider-repl-type>
"cljs"

<send some cljs to the repl>
*cider-repl--state-handler called on the nrepl op return*

<cljs-repl-type>
"clj"

This is not what I would expect. To be honest I haven't tried other repls, but
I will.

In the meantime, any thoughts?

[vspinu]

I think this is some kind of legacy code which no longer serves its purpose. AFAIC judge, It doesn't make much sense to allow nrepl to change repl's type.

[bbatsov]

@vspinu Actually it's not legacy at all, it's just not working quite right presently. After each eval operation we try to infer if the user didn't do something that would change the REPL type (which we can't really know without tracking it somehow). That allows for the REPL type to be auto-adjusted when needed - e.g. after cider-connect, after you type :cljs/quit, etc.

The relevant code is here https://github.com/clojure-emacs/cider-nrepl/blob/master/src/cider/nrepl/middleware/track_state.clj#L172

Clearly the code doesn't work properly, which is a the root cause of this problem. This doesn't invalidate the need for this, though.

Also - in general we should enhance describe-session to show the repl-type as well, as this would simplify setting the type on connect. Right now even with this middleware the repl type might be incorrect until the first eval message.

[bbatsov]

I think the right (temporary) solution would be to just make this configurable. Something like "cider-auto-track-repl-type". Or figure out why the cljs env check is failing in some cases.

[bbatsov]

This is not what I would expect. To be honest I haven't tried other repls, but
I will.

You likely just have to take a look at (cljs/grab-cljs-env msg). Probably it checks something that doesn't exist in shadow or whatever.

[vspinu]

after cider-connect, after you type :cljs/quit, etc.

Forgot about that. Indeed, should have been obvious - cljs environments work by "upgrading" the connection.

Right now even with this middleware the repl type might be incorrect until the first eval message.

To avoid such confusion, cider-create-repl should not set repl-type then. The only source of the type change should be the state-change callback. Note also that cider-repl-set-type doesn't change the buffer name at the moment. I will take care of these issues in the forthcoming connection API PR.

[arichiardi]

Thanks these are all great pointers. Now I understand how things work a bit more and can debug.

I like the idea of the auto custom though, sometimes you really want to get it to work without exploring why it is not working. Workaround I know...but practical.

[bbatsov]

The only source of the type change should be the state-change callback.

Ideally yes.

Note also that cider-repl-set-type doesn't change the buffer name at the moment. I will take care of these issues in the forthcoming connection API PR.

An oversight of mine. Thanks for tackling it.

I like the idea of the auto custom though, sometimes you really want to get it to work without exploring why it is not working. Workaround I know...but practical.

That's a fair point. I'm OK with adding a workaround, but I'd rather wait to see what @vspinu is going to come up with first.

[arichiardi]

Ok let's wait for it then, I am ok with having this in my fork for now.

[bbatsov]

OK, now that the connection changes are in we can add the config for this.

[arichiardi]

I don't know why yet but this seems to work fine now. Closing until further investigation