Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New connection manager reads the wrong cider-default-cljs-repl #2336

Closed
arichiardi opened this issue Jun 18, 2018 · 11 comments
Closed

New connection manager reads the wrong cider-default-cljs-repl #2336

arichiardi opened this issue Jun 18, 2018 · 11 comments

Comments

@arichiardi
Copy link
Contributor

Expected behavior

When I call C-c M-J the cider-connect-sibling-cljs should read the cider-default-cljs-repl from the code buffer's locals.

Actual behavior

It always reads from the REPL buffer.

Steps to reproduce the problem

Have a .dir-locals.el like this in your folder:

((nil . ((cider-default-cljs-repl . figwheel-main)))

C-c M-J in a code buffer. You will always be prompted no matter what is the value of cider-default-cljs-repl.

CIDER version information

latest master

The problems seems in here, the variable should be read from the code buffer but it is read from the REPL instead.

@vspinu
Copy link
Contributor

vspinu commented Jun 18, 2018

Good catch. Will fix in the evening.

@bbatsov
Copy link
Member

bbatsov commented Jun 18, 2018

Just noticed a similar issue here - https://github.com/clojure-emacs/cider/blob/master/cider.el#L973

When you're connecting to a cljs repl you don't really need to select it's type, as someone already has converted a to a cljs repl, before you connected to it.

@vspinu
Copy link
Contributor

vspinu commented Jun 18, 2018

Are you sure? How that could happen in concrete terms?

I think every connection starts as a clj connection and then upgraded. I am not connecting to a REPL which someone already converted.

@arichiardi
Copy link
Contributor Author

I think @vspinu is right and I do this all the time:

  • launch a Clojure REPL in the terminal with all the required deps
  • cider-connect
  • execute forms for upgrading

I think cider-connect-cljs does the last two steps for me automatically?

@vspinu
Copy link
Contributor

vspinu commented Jun 18, 2018

I think both situations can be. The REPLs connected to the same server share state. So, if you upgrade into cljs with one REPL you will automaticallly get cljs in the other repl. So the solution here would be to first check if the connection already running cljs, an if not upgrade.

In this regard clj/cljs is the type of the connection, not a repl. We need to rename a few variables and functions then :)

@arichiardi
Copy link
Contributor Author

arichiardi commented Jun 18, 2018

I think both situations can be. The REPLs connected to the same server share state. So, if you upgrade into cljs with one REPL you will automaticallly get cljs in the other repl.

@vspinu it is not necessarily true, for example I think shadow-cljs has independent "workers" and you can connect and upgrade kind of independently. Let's ask @thheller more info though.

So the solution here would be to first check if the connection already running cljs, an if not upgrade.

Brainstorming here, maybe if the session and the REPL type match maybe we do not upgrade?

All this if we want to support REPLs that are independently handling sessions.

@thheller
Copy link

shadow-cljs can run multiple builds in parallel and each build can have its own independent REPL which you can connect/disconnect/reconnect at any time.

I wrote sort of a braindump recently here: nrepl/piggieback#88

I don't know what the connection manager is about but dealing with multiple runtimes is a problem not present in Clojure. In practice there should be a Build select and a Runtime select (if multiple are connected).

Most of the tooling doesn't actually require a REPL too just a compiler-env which could be spawned entirely independently and without user config.

Sorry, too busy to get into this at the moment. Just thought I mention this since it easy to forget that CLJS has slightly different mechanics than CLJ.

@vspinu
Copy link
Contributor

vspinu commented Jun 18, 2018

From what I understand right now, the current implementation is correct for standard style piggyback upgrades. Check it for yourself, jack-in into cljs then connect-clj or connect-cljs to it.

nREPL Clj REPLS share same environment (JVM Runtime in @thheller terminology?). Thus, multiple connections (repls) will share state (namespaces, vars etc.). But cljs upgrade creates cljs environments which are insulated bubbles (Cljs Runtimes in @thheller terminology - browsers, react-native emulators etc).

In current implementation of piggiback each upgrade creates IJavaScriptEnv in which cljs code is evaluated. Thus, cljs repls are independent and cannot share state.

Within the same emacs session multiple clj repls into same server are seldom needed. One use case is when one REPL runs a time-consuming job and you still want to work on the project in the meanwhile. In this case, shared state is useful.

Multiple cljs REPLs make more sense from CIDER's prospective. You can run nashorn, node, etc simultaneously (Builds in @thheller terminology?). Each Build can provide multiple connections to Runtimes. From cider's prospective we don't really care. We have a bijection - each Runtime is a repl - and how those REPLs share state, communicate with each other, communicate with the Build, or even which build created which runtime is not our concern.

For instance if shadow-clj or figwhell can provide multiple connections to, say Chrome, Firefox etc, and all of those would share state, that is fine with us, we will have several REPLs sharing state just like clj repls do. But, it's hard to imagine how one would implement sharing state between for instance Firefox SPA and Chrome SPA runtimes.

On the CIDER side, an illusion of shared state happens as a consequence of simultaneous evaluation of the code into multiple runtimes, same code => same state.

In any event, it feels to me that what we have right now is the right answer to the plehora of different solutions out there.

@bbatsov
Copy link
Member

bbatsov commented Jun 18, 2018

That's a nice summary of the situation, but I still worry about the change to cider-connect-cljs.

The previous cider-connect-cljs was just connecting an already running cljs repl. I get this new behaviour can be useful, but generally the assumption with cider-connect has been that you connect the REPL and you just start using it as is.

Previously the only difference between the regular connect and cljs connect was the second one was changing the type of the repl to cljs.

I'll have to think about the optimal behaviour for this, but at the very least there should be some check whether the cljs upgrade is needed or not.

@arichiardi
Copy link
Contributor Author

I'll have to think about the optimal behaviour for this, but at the very least there should be some check whether the cljs upgrade is needed or not.

For what is worth, to me this feels the best solution

@vspinu
Copy link
Contributor

vspinu commented Jun 18, 2018

Previously the only difference between the regular connect and cljs connect was the second one was changing the type of the repl to cljs.

Looks like previously it was working incorrectly on two accounts. First, the connected repl was clj, second it was set wrongly to cljs type. You can revert and see this for yourself with pre conn-api version:

## 1
lein repl
## 2 
(do (require 'cljs.repl.node) (cider.piggieback/cljs-repl (cljs.repl.node/repl-env)))
## 3 
# from emacs do M-x cider-connect-cljs

After this I get a clj repl with cljs buffer name and CLJS mode line. So it either was never working correctly or I miss something fundamental here. What I am insisting on is that even if you upgraded in a remote repl to cljs, new connection won't see the upgrade. I am not familiar with piggyback internals but this behavior makes sense to me. You connect to a server not to a REPL. Server doesn't know that you upgraded to cljs repl-env, or does it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants