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

Add support for pausing/resuming a CLJS REPL #73

Open
cemerick opened this Issue Jun 2, 2017 · 15 comments

Comments

Projects
None yet
6 participants
@cemerick
Copy link
Collaborator

cemerick commented Jun 2, 2017

(Originally proposed in #28, which couldn't be applied due to the 0.2.0 rewrite landing.)

Basic outline:

  1. Once in a CLJS REPL, :cljs/pause would be intercepted and shift the REPL environment over to a different dynamic var, thus restoring the nREPL session to Clojure "mode".
  2. In this state, :cljs/resume would restore the REPL environment, putting the session back in ClojureScript "mode".

Should be pretty easy, and make using piggieback a lot easier, i.e. make it less necessary to have two nREPL sessions going in a lot of cases (one Clojure, the other piggiebacked CLJS).

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Mar 5, 2018

Yeah, that'd be pretty cool!

@arichiardi

This comment has been minimized.

Copy link

arichiardi commented Mar 14, 2018

This seems pretty similar to @cgrand concept of unrepl upgrade, I might be wrong though so bringing this up for visibility only.

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Mar 14, 2018

Well, it's basically an op rebinding a few dynamic vars (you can see the original proposal for 0.1 here 36efdda). Not sure if @aaronc is interested in updating his PR for 0.2, but I think we should certainly pursue this as it'd benefit all nREPL clients a lot.

@cgrand

This comment has been minimized.

Copy link

cgrand commented Mar 14, 2018

@arichiardi modal repls were discussed months ago with @cursive-ide who thought they have poor usability.

@bhauman

This comment has been minimized.

Copy link
Collaborator

bhauman commented Mar 14, 2018

This idea is interesting, I have another idea in the pipeline, a :passthrough attribute that when set to true passes the eval op down to the interruptible eval while ensuring the cljs.env/compiler (and the new cljs.repl/repl-env) is bound in the environment.

This would give cljs tooling a very simple way of using the connection if needed and it would have all the information it needs available (including a way to easily eval in the js env client).

@cgrand

This comment has been minimized.

Copy link

cgrand commented Mar 14, 2018

Why not consider a special form/taglit (or abuse of conditional reader) to determine in which env should a piece of code be run?

@bhauman

This comment has been minimized.

Copy link
Collaborator

bhauman commented Mar 14, 2018

Abusing the conditional reader is a very interesting solution.

@bhauman

This comment has been minimized.

Copy link
Collaborator

bhauman commented Mar 14, 2018

In fact, we could set it up so that it's not really abuse, it's just that an implied conditional is set for code without conditionals.

@arichiardi

This comment has been minimized.

Copy link

arichiardi commented Mar 14, 2018

I was sure some cool idea would come out of this 😄

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Mar 14, 2018

That sounds like an amazing idea! Would be game changing for a tool like CIDER.

@bhauman

This comment has been minimized.

Copy link
Collaborator

bhauman commented Mar 14, 2018

Practically speaking in the short term, this means top level reader conditionals of the form #?(:clj (print "hello")) would cause piggieback to unwrap and forward the eval to the clojure eval.

@cgrand

This comment has been minimized.

Copy link

cgrand commented Mar 15, 2018

In fact, we could set it up so that it's not really abuse, it's just that an implied conditional is set for code without conditionals.

That's exactly what I meant by abuse: considering an implicit conditional when none.

@thheller

This comment has been minimized.

Copy link

thheller commented Mar 23, 2018

I already mentioned my reservations about this in the #cider slack but let me repeat it here so it doesn't get lost. The problem with :cljs/resume is that there may be many suspended CLJS REPLs. It is very common for bigger CLJS projects to have multiple builds. Say you have :server, :web, :app builds representing a node.js server, a browser-targetted SPA and a react-native app all in the same codebase.

In shadow-cljs these can all run in parallel and you can "select" which REPL to talk to at any time.

(shadow/nrepl-select :server) lets me talk to the server REPL and :cljs/quit drops the nREPL session back down to CLJ. (shadow/nrepl-select :web) lets me then switch to the browser REPL and so on.

:cljs/quit in shadow-cljs doesn't actually quit the REPL, it just disconnects the client portion but keeps all state. So if you (shadow/nrepl-select :server) -> ... -> :cljs/quit -> (shadow/nrepl-select :server) you will resume where you left off.

:cljs/resume is problematic because a) it need to intercept eval and b) it is not parameterized. It doesn't say which one it should resume. Say I start :server -> :cljs/pause -> :web -> :cljs/pause. How do I get back to :server without going through :web?

I suggested to use nREPL ops for this since :op "cljs/pause" could return an identifier which you could then send with :op "cljs/resume" :id <that-id>.

The broader problem however is that ClojureScript is not like Clojure and maybe we should re-think some assumptions about the REPL. One issue is that the Read runs in a different runtime than Eval/Print. For the Browser REPL it is very possible (and common) that the user reloads the Browser which means all state that might have been accumulated is lost. The compiler-env however is still around so the compiler thinks state exists when it doesn't.

[7:1]~cljs.user=> (def x 1)
#'cljs.user/x
;; browser reloads. should this maybe end the REPL?
[7:1]~cljs.user=> (inc x)
##NaN
;; should have warned about x not existing

You also can't Read inside Eval since JS doesn't have a blocking streaming stdin (or stdin at all in the case of the Browser).

And since there are different runtimes for each build most of the tooling is in a weird situation. You might be editing my/app/web.cljs but still be connected to the :server REPL which doesn't have this namespace loaded or even compiled (because its only used by the :web build). It is very possible that the :server REPL can't load this namespace if it contains some DOM interop or some other browser-only things. It might not even be possible to compile this namespace if the :server config is missing something essentials that are only configured for :browser.

There is also this weird and common issue that a REPL might actually have multiple runtimes connected. Say you have the :web build opened in Chrome and Firefox at the same time. Who gets to eval? Both? Whose result do you display? figwheel currently just displays the first result it gets I believe. In shadow-cljs I disallow eval completely because its impossible to ensure that both runtimes are in the same state
since they may have connected at different times. There should be an option to select which runtime to eval in but this is not represented in any tooling at all.

This went way off-topic so I'll stop here.

PS: I'm absolutely not a fan of abusing reader conditionals for this, that just seems like a hack gone too far. The client knows if it wants to eval CLJS or CLJ. It should tell this to the nrepl middleware via either different ops or extra attributes in the message.

@arichiardi

This comment has been minimized.

Copy link

arichiardi commented Sep 19, 2018

Re-reading this last post multiple times. It makes a lot of sense to me to try to come up with a cljs-specific nREPL mini-protocol. It could be handled by its own middleware for achieving isolation and could be initially behind feature flag so that we do not break the existing behavior.

For me all the above makes sense as I have directly experienced all of the problems above (especially the namespace not compiled one!).

I will see if I can come up with some simple stuff that brings together ideas spread in there issues.

@bhauman

This comment has been minimized.

Copy link
Collaborator

bhauman commented Sep 19, 2018

@arichiardi :passthrough-eval true is the most general and simplest solution here

I'm not a believer in a mini protocol, it's coupled and specific and would require an every growing set of operations. Almost everything that needs to be done can be done with a clojure library with :passthrough-eval true which will allow you to eval Clojure in the same session with the compiler-env bound.

I do think suspend and resume are good ideas and contrary to the post above you can resume because you are keeping track of the session ids. :suspend and :resume are partners to the idea of :passthrough-eval true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment