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

Double (Clojure + ClojureScript) client REPLs #1195

Merged
merged 8 commits into from
Jul 13, 2015
Merged

Double (Clojure + ClojureScript) client REPLs #1195

merged 8 commits into from
Jul 13, 2015

Conversation

Malabarba
Copy link
Member

First, one needs to add these to projec.clj (as described on the Readme):

:dependencies [[com.cemerick/piggieback "0.2.1"]
               [org.clojure/tools.nrepl "0.2.10"]
               [org.clojure/clojure "1.7.0"]]
:repl-options {:nrepl-middleware [cemerick.piggieback/wrap-cljs-repl]}

and then invoke M-x cider-jack-in-clojurescript. This should create two repl buffers.

@bbatsov
Copy link
Member

bbatsov commented Jul 11, 2015

Overall things are looking good.

The cider-cljs-repl-code variable, in particular, should probably be made into middleware.

Probably. It's possible values could simply enumerate the types of REPLs supported by piggieback.

The presentation of connection info should also be augmented to show whether something is clj/cljs. Probably the same goes for the REPL buffer names.

@Malabarba
Copy link
Member Author

Probably. It's possible values could simply enumerate the types of REPLs supported by piggieback.

I like that, and I've settled on that for now. I've also added that to the Readme.

The presentation of connection info should also be augmented to show whether something is clj/cljs. Probably the same goes for the REPL buffer names.

Done.

I think this looking nice now. I'm gonna test it out a bit more, but this might be good to merge.

@Malabarba
Copy link
Member Author

I played around a bit and things seem to be working.
The only issue is that the rhino repl is the only one that works for me. The Node repl which we recommend in readme didn't work for me (it threw some exceptions and resulted in a cljs REPL that just kept throwing exceptions).

@bbatsov
Copy link
Member

bbatsov commented Jul 12, 2015

The Node repl which we recommend in readme didn't work for me (it threw some exceptions and resulted in a cljs REPL that just kept throwing exceptions).

You do have nodejs installed, right? :-) I think only rhino works out of the box, because rhino is part of the JVM.

@cichli
Copy link
Member

cichli commented Jul 12, 2015

@Malabarba thanks for looking into this - I've thought about adding support for this in the past but never got around to actually doing it :-).

  • Some projects (for example, those that use figwheel) might not invoke cemerick.piggieback/cljs-repl directly to start a REPL - we should just include the call to it in thecider-cljs-repl custom
  • Additionally, some projects are single language (either CLJ or CLJS), but would still benefit from two sessions/REPLs for files in different parts of the class path (e.g. src/server/ and src/client/).

Could we have a list of '(session-name filespec init) triples to open this up to configuration? e.g.

'((clj "*.clj" nil)
  (cljs "*.cljs" "(cemerick.piggieback/cljs-repl ..."))

We would then just prompt for the session to start when invoking cider-jack-in (rather than hardcoding a specific command for CLJS), and have a similar command for starting a new session on top of an existing connection. Thoughts?

@bbatsov
Copy link
Member

bbatsov commented Jul 12, 2015

Additionally, some projects are single language (either CLJ or CLJS), but would still benefit from two sessions/REPLs for files in different parts of the class path (e.g. src/server/ and src/client/).

I don't quite understand what you mean.

We would then just prompt for the session to start when invoking cider-jack-in (rather than hardcoding a specific command for CLJS), and have a similar command for starting a new session on top of an existing connection. Thoughts?

What exactly are we going to ask the user for? Is this config going to be something in the projects themselves?

@Malabarba
Copy link
Member Author

@bbatsov

You do have nodejs installed, right? :-)

Oh, that might be it. 😅

@cichli

Some projects (for example, those that use figwheel) might not invoke cemerick.piggieback/cljs-repl directly to start a REPL we should just include the call to it in the cider-cljs-repl custom

Good to know. I'll do that then.

Could we have a list of '(session-name filespec init) triples to open this up to configuration? e.g.

I like the idea of the variable. I don't think we need to prompt the user. They can configure it as a dir local variable for the project, and if jack-in detects this variable is configured it automatically creates the specified REPLs (once the nrepl server has been started, creating REPL sessions is fast).
That said, the current PR is large enough as it is. Can we file that as a separate issue?

@bbatsov
Copy link
Member

bbatsov commented Jul 13, 2015

Overall I think the current state is good enough to merge when the small outstanding issues are addressed. We'll continue to refine this functionality down the road. I can envision other small checks - e.g. verify Clojure is 1.7+, if you select a node repl check you have node, etc.

@Malabarba
Copy link
Member Author

Some projects (for example, those that use figwheel) might not invoke cemerick.piggieback/cljs-repl directly to start a REPL - we should just include the call to it in thecider-cljs-repl custom

Done now.

We'll continue to refine this functionality down the road

Agreed. With should open an issue with the points discussed here.

@bbatsov
Copy link
Member

bbatsov commented Jul 13, 2015

There's a failing test.

@Malabarba
Copy link
Member Author

Yeah I see it. Pushed againnow...

bbatsov added a commit that referenced this pull request Jul 13, 2015
Double (Clojure + ClojureScript) client REPLs
@bbatsov bbatsov merged commit ae8cda3 into clojure-emacs:master Jul 13, 2015
@bbatsov
Copy link
Member

bbatsov commented Jul 13, 2015

OK, the first step was made. Now it's refinement time.

@Malabarba Malabarba deleted the double-client branch July 13, 2015 11:52
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

Successfully merging this pull request may close these issues.

3 participants