non-eval ops shouldn't use tooling session, now specifically "complete" for the sake of nrepl-complete middleware

[gtrak]

I had a long discussion with @cemerick about alternative approaches for autocomplete in cljs w/nrepl.

The driving concerns:
The piggieback middleware only adds the clojurescript compiler environment to the session, on the main session.

nrepl-complete can only access the compiler environment if it's been added by piggieback. See https://github.com/gtrak/nrepl-complete/blob/master/src/nrepl_complete/middleware.clj#L9 and https://github.com/cemerick/piggieback/blob/master/src/cemerick/piggieback.clj#L238

As I understand it, the drive for a separate tooling session is to avoid botching env variables (*1 *e, etc.) or blocking user "eval". Nrepl complete uses the "complete" op, and it can never mess up those env vars.

I don't see a way to associate the needed environment of the main session to the tooling session cleanly.

Assuming we stick with the tooling session, I'd need a way to grab the needed data through a global mechanism, which would require knowing the ID of the main session. I don't think it makes sense to pass the ID of the main session into every op, like a :target-session key or something. That would be painful at scale. If there were a general way to associate sessions, I'm not sure why it would conceptually make sense.

This hasn't been a problem before because multiple clojure nrepl sessions are coupled to each other implicitly through JVM state. CLJS and piggieback break that assumption, and in general, we want decoupling and removal of assumptions from the client.

A nrepl op middleware approach would remove the burden of bookkeeping from the client to the extent that they're implemented.

[gtrak]

@bbatsov ^

[bbatsov]

@gtrak The complete middleware code looks good. I'm OK with dispatching the op in the main session for now. I think we should include the middleware in the cider-nrepl project (which should in time become the home of all essential cider middleware).

[johnbendi]

@gtrak We need to think about when we need cljs completions. And that's when we are in a cljs repl. Right? If that's so then at that point the cljs-env has taken position. Don't concern yourself with that. Just do the cljs completions part and we are immediately okay. For clojure completions we already have clojure-complete which immediately takes over on :cljs/quit.

So something like (if (and (@session #'*cljs-repl-env*) (= op "complete")) ...) should do the magic. wrap-cljs-repl in piggieback.clj has a test like that.

[gtrak]

I'm not sure I understand this.

What's 'taken position'?, the implementing code for cljs-complete is
actually jvm-clojure code working on the state of the cljs compiler (which is stored in a var in jvm-clojure).

Clojure-complete is insufficient only because the cider client impl uses eval to call it directly instead of through a nrepl op. This is effectively coupling cider to a transitive dependency of REPL-y, which I think is kind of backwards. This should really be an nrepl op.

We can't get to cljs completion information through a cljs eval call AFAIK. CLJS doesn't have namespaces as objects that we can access in the same way as Clojure namespaces.

I'm already doing a check like piggieback's to determine whether to do clojure or cljs completion. I've tested both.

On Sat, Jan 25, 2014 at 1:20 PM, John Chijioke notifications@github.comwrote:

@gtrak https://github.com/gtrak We need to think about when we need
cljs completions. And that's when we are in a cljs repl. Right? If that's
so then at that point the cljs-env has taken position. Don't concern
yourself with that. Just do the cljs completions part and we are
immediately okay. For clojure completions we already have clojure-complete
which immediately takes over on :cljs/quit.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/460#issuecomment-33295734
.

[gtrak]

I should also mention, if ever there's an alternate strategy to getting at the CLJS compiler state, we'd need to update the middleware to reflect that because right now we're relying on piggieback implementation details.

[bbatsov]

@gtrak Just add the middleware here https://github.com/clojure-emacs/cider-nrepl and update the cider completion logic as we discussed on the mailing list. This very important feature has been delayed long enough I think.

You should also add a section in the README regarding ClojureScript support and the need to install the completion middleware.

[gtrak]

@bbatsov will do.