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

Draft: specifying default connection params #3359

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yuhan0
Copy link
Contributor

@yuhan0 yuhan0 commented Jun 25, 2023

Context: https://clojurians.slack.com/archives/C0617A8PQ/p1687640686417379

As discussed on Slack, a naive clj-only implementation of a feature which allows the user to skip cider-connect prompts asking for the hostname and port.

The intended usage here is to set cider-default-connect-params explicitly per project as a dir-local variable, which is why it's a defvar-local instead of defcustom.

Eg. I have a project which specifies in its deps.edn

{:aliases {:dev 
   :main-opts ["-m" "nrepl.cmdline" "--port" "2171" ,,,],,,},,,} 

so running clj -M:dev in an external terminal always starts a Cider nrepl server on the port 2171.

Then in the project's .dir-locals.el:

((clojure-mode
  . ((cider-connect-default-params . (:host "localhost" :port 2171)))))

Result: M-x cider-connect on any clj buffer within that project skips the prompts entirely, saving the friction of a couple of RET RET keypresses on every connect. Which can be quite frequent when investigating project crashes (or hacking on Cider itself)


Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

@yuhan0 yuhan0 marked this pull request as draft June 25, 2023 13:20
@yuhan0
Copy link
Contributor Author

yuhan0 commented Jun 25, 2023

Couple of downsides / tradeoffs with the above:

  • No Clojurescript support, mainly because I don't work with cljs and am not sure what the nuances are there. The definition of cider-connect-cljs directly following it looks similar to connect-clj but do we apply the same common params to both? Have 2 separate vars for clj and cljs? Turn it into an alist? ((:clj . (:port 1234)) (:cljs . (:port 5678)))

  • Explicit hard coding of the values, eg for port number which Cider can already detect in .nrepl-port etc. files . Maybe there would be an :detect option, to basically mean "just auto select what would be the first choice in the prompt". Not sure of any nuances aroundhost, where we've only ever used localhost.

Note: in the above scenario, if the port 2171 doesn't exist, the command throws a "No server found" error - which I very much prefer over being presented with a empty completion list, and having to take a second or two to interpret it. ("Oh, I haven't started the server yet, and Cider couldn't detect any port files, that's why I'm seeing a lack of something")

@yuhan0
Copy link
Contributor Author

yuhan0 commented Jun 25, 2023

So this particular implementation suits our workflow where each project's dev REPL has its own "identifiable" static port number, instead of being assigned a random one by cider-nrepl. Port clashes are a feature because we don't want two simultaneous dev servers of the same project started by accident.

Might it be reasonable to assume similar practices for users who choose to cider-connect instead of cider-jack-in? I've always seen the random auto-generated ports as a jack-in convenience, whereas "real" external servers would usually want to specify a static port that their clients could reliably point to, eg. in config files.

@bbatsov
Copy link
Member

bbatsov commented Jun 26, 2023

Note: in the above scenario, if the port 2171 doesn't exist, the command throws a "No server found" error - which I very much prefer over being presented with a empty completion list, and having to take a second or two to interpret it. ("Oh, I haven't started the server yet, and Cider couldn't detect any port files, that's why I'm seeing a lack of something")

That'd be fine by me. I prefer explicit errors.

A general note - If I recall correctly CIDER currently extracts the port number from the .nrepl-port or similar file, so I guess in the command is started from a folder with such a file this can treated like .dir-locals. One edge case I can think of is trying to connect to another server while in the folder of some project with some connection configuration, but I guess that's not that big of a deal.

@vemv
Copy link
Member

vemv commented Jun 26, 2023

I was thinking, maybe given that cider-connect-clj already accepts params, it's easy enough to either advice-add it at will, or code a wrapper? Then one would M-x my-connect and be done.

This would save us the few possible complications detected in the discussions (GH + Slack)

@bbatsov
Copy link
Member

bbatsov commented Jul 2, 2023

I still think that's not a big deal, provided there's an easy way to ignore the default connection params. And most of the time you'd want to connect automatically to some nREPL endpoint, so that'd be a productivity improvement IMO (although a small one).

@yuhan0
Copy link
Contributor Author

yuhan0 commented Jul 2, 2023

provided there's an easy way to ignore the default connection params

I think this PR implements something like it - use a prefix arg / C-u before cider-connect and it will prompt for the params.

Feel free to take or adapt the changes here in any way for the Clojurescript side of things - as mentioned on the Slack this PR was just a code reference for a "Can CIDER do this?" question that I've already hacked together on a local fork.

(From my understanding the advice mechanism is mostly a last resort for modifying external packages out of our control, probably shouldn't be used in this case)

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 7, 2024

Resolved conflicts with the master branch - any interest in this PR and/or generalizing it to work with CLJS commands?

@yuhan0 yuhan0 marked this pull request as ready for review May 7, 2024 12:51
Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any interest in this PR and/or generalizing it to work with CLJS commands?

Sure, let's go for it. Having cljs from the beginning would seem important, else it could get confusing/broken shortly thereafter.

Thanks!

cider.el Outdated
@@ -1504,14 +1504,21 @@ server buffer, in which case a new session for that server is created."
(plist-put :session-name ses-name)
(plist-put :repl-type 'cljs)))))

(defvar-local cider-connect-default-params nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to have two variables - cider-connect-default-clj-params, cider-connect-default-cljs-params.

Commands like cider-connect-clj&cljs would use the former for the clj part, the latter for the cljs part (which would seem cleaner than a third variable cider-connect-default-clj&cljs-params)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(replied in the main thread below by accident, feel free to continue conversation here)

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 7, 2024

As far as I can tell, the only difference in params between connect-clj and connect-cljs is the :cljs-repl-type key, which informs cider--update-cljs-init-function how to assoc a bunch of other keys to the params map like :repl-init-form and :repl-init-function.
Would it make sense for all three commands to share a common defcustom, with the clj half simply ignoring or dissoc'ing the irrelevant key? Empirically, attempting to cider-connect-clj with a param map containing '(:clj-repl-type nonsense) already works without issue.

I'm not familiar with clojurescript dev workflows, maybe there are instances where one would have different nREPL hosts / ports for CLJ and CLJS repls? (noting that the clj&cljs command reuses the same host and port for the sibling cljs repl without double-prompting)

@vemv
Copy link
Member

vemv commented May 7, 2024

It's reasonably frequent to have different ports. There's also the case of babashka (we sometimes use 'cljs' to denote 'bb'), where definitely there are different ports being used for the vanilla JVM and the bb process.

Either way, I'd try to not be too smart here - leaving everyone with something easy to understand/maintain.

...I especially wonder about case of accidentally connecting with cljs params to a clj repl or vice versa. Potentially very confusing.

command reuses the same host and port for the sibling cljs repl

Good catch!

It seems like the right thing would be to use the cider-connect-default-clj-params params for both connections, if cider-connect-default-cljs-params is unset. This honors traditional behavior (which is a bit coupled IMO, but regardless it shouldn't be broken).

If both vars are set, honor them individually, respectively.

@bbatsov
Copy link
Member

bbatsov commented May 7, 2024

That makes sense to me as well.

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 7, 2024

Hmm, it appears that there's an existing defcustom cider-default-cljs-repl, whose docstring states

This affects commands like `cider-jack-in-cljs'.   Generally it's
intended to be set via .dir-locals.el for individual projects [...]

It's not explicitly mentioned, butcider-connect-cljs does respect this defcustom, threading the param map through cider--update-cljs-type and skipping the user prompt if the variable is non-nil.

This points to a possible conflict - what would be the desired outcome of the following dir-locals.el?

((clojure-mode 
  (cider-connect-default-cljs-params . (:host "localhost" :port 8000 :cljs-repl-type figwheel))
  (cider-default-cljs-repl . shadow))))

@vemv
Copy link
Member

vemv commented May 7, 2024

Good eye!

cider-default-cljs-repl is a defcustom. It seems you've done better by using defvar-local.

Setting defcustoms from .dir-locals is also slightly fragile - in the end it's a global side-effect with no guarantees of being wiped as one switches dirs.

So it seems reasonable to let the new var win.

@bbatsov
Copy link
Member

bbatsov commented May 7, 2024

Setting defcustoms from .dir-locals is also slightly fragile - in the end it's a global side-effect with no guarantees of being wiped as one switches dirs.

I'm not sure what you mean by this. Everything set via .dir-locals.el becomes local in effect. The only practical difference is whether a global setting for something makes sense or not. I can argue it makes more sense for a default ClojureScript REPL than for connection params.

And I agree that probably the connection params should override the default REPL setting.

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 7, 2024

There's also the case of babashka (we sometimes use 'cljs' to denote 'bb')

I wasn't aware of this, could you help me understand where such a situation would occur? I'm familiar with using Babashka as a scripting language, but this involves cider-jack-in-clj / connect-clj with babashka as the project type (auto-detected or user-specified via the cider-preferred-build-tool defcustom)

@bbatsov
Copy link
Member

bbatsov commented May 7, 2024

I think it's actually the other way around, as some ClojureScript platforms (e.g. nbb) will work via cider-connect-clj (as noted in https://docs.cider.mx/cider/platforms/other_platforms.html)

@vemv
Copy link
Member

vemv commented May 7, 2024

I'm not sure what you mean by this. Everything set via .dir-locals.el becomes local in effect.

True (except for the case of wrapping setq with eval - but should be rare)


Fairly certain of a case of 'cljs' meaning 'bb' - will check later today. I think it was related to the repl-type.

@vemv
Copy link
Member

vemv commented May 7, 2024

But regardless, one should be able to have two independent connections for any combination of clj|cljs|bb|nbb|... with clj|cljs|bb|nbb|....

So having two 'slots' that are known to be independent (not merged, assoced, dissoced) would only help.

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 7, 2024

Please correct me if I'm mistaken, but does this mean that Cider's notion of a clj vs cljs repl is tied to the paradigm of 'upgrading' a JVM repl by sending some sort of initialization code to it, putting it into a sort of nested REPL loop which communicates with the browser / javascript platform, and only then exposing this to the user?
i.e. something that was traditionally the case but no longer holds for impls like nbb, hence the confusion of terminology?

@vemv
Copy link
Member

vemv commented May 7, 2024

Probably. For full-stack-Clojure apps using shadow-cljs, the most common setup would be to spin up different JVMs for the backend and for the frontend.

(I'd enjoy the most having a single JVM, but last time I asked, going over npx is the most supported route)

Which is to mean, things have changed, not only with bb and friends but also within the cljs ecosystem.

(another cljs thing that is less relevant today is Piggyback)

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 7, 2024

Stepping back for a moment, the motivation here is really just convenience - the user has some known external means of launching one or more REPLs, at some fixed TCP ports / sockets, and wants some means to inform Cider of this fact in order to skip the small annoyance of pressing RET twice on every cider-connect.

We see the same principle in existing defcustoms like cider-default-cljs-repl and cider-preferred-build-tool – "Yes I have a project.clj and deps.edn in the project root, please stop prompting me every time I try to jack-in and just select one of them"

This PR simply extends this to the "Select host" and "Select port" prompts for a cider-connect-based workflow. Possibly this also helps with the use-case of some ambiguous :project-dir - eg. in a Polylith directory, where you might want to treat either the component's subdir or project root dir as the place where Cider should look for a .nrepl-port file.

For full-stack-Clojure apps using shadow-cljs, the most common setup would be to spin up different JVMs for the backend and for the frontend.

Thanks for this pointer - perhaps I should clone a simple starter shadow-cljs app to better understand how this would work. Say the user executes npx shadow-cljs server in their terminal, I'm guessing this would spin up 2 JVM processes with separate nREPL ports, and the connection sequence from Emacs would go something like the following?

(in a clj buffer)
M-x cider-connect-clj
RET (select localhost)
RET (select 1st port picked up from <root>/.nrepl-port)
(switch to a cljs buffer)
M-x cider-connect-cljs
RET (select shadow - can already skip with existing defcustom)
RET (select localhost)
RET (select 2nd port picked up from <root>/.shadow-cljs/nrepl.port)

The goal here is simply to reduce the number of RET's required from the user, via the minimal customization spec in the dir-locals needed to inform CIDER of their intention.

@vemv
Copy link
Member

vemv commented May 7, 2024

Stepping back for a moment, the motivation here is really just convenience

Yes, but adding that convenience shouldn't introduce or amplify coupling, assumptions, etc.

In the end, I believe the provided feedback is very simple to implement.

It's also very easy for users to use. Data, unlike code, tends to be cheap to duplicate with no consequences.

In specific terms, I much prefer that users have to specify the port twice in .dir-locals.el (it's data, it's cheap) than having us couple things here (it's code, it's expensive).


Re: shadow-cljs what I consider a typical full-stack workflow is:

  • for the backend, create a JVM process as usual with your tool of choice
  • for the frontend, create a shadow repl with npx, which will spin up one JVM

So we have two JVMs in total.

Not all Clojure apps are full-stack-Clojure, and even then, there are workarounds (like letting shadow-cljs manage the backend JVM s well) for keeping it single-JVM.

tldr there are many scenarios. By keeping things decoupled, all use cases fit into the equation.

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 7, 2024

Sure, that sounds reasonable :)

On an (even more meta) preference level, I feel like CIDER already suffers from a sort of proliferation too many overlapping and confusingly-named customization options, thus I was a little hesitant about adding to it, eg. by carelessly introducing variant -clj/-cljs defcustoms when a single one would suffice.

In this case there appear to be good reasons for decoupling them, thanks for spending the time to clarify why 🙇

Re. naming, how about

cider-connect-default-params (since this applies to both clj and cljs)
cider-connect-default-cljs-params (overrides the former if non-nil)

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 7, 2024

I also considered putting "default" first, in line with cider-default-cljs-repl, ie. something like

cider-default-clj-connection-params
cider-default-cljs-connection-params

But the first pair makes it clearer that they apply only to the cider-connect set of commands.

We could also omit it altogether, in line with many other defcustoms like cider-clojure-cli-aliases which are also intended for dir-locals:

cider-connect-(cljs-)params / cider-(cljs)-connection-params

Or even use "preferred" like in cider-preferred-build-tool:

cider-preferred-(cljs-)connection-params / cider-connect-(cljs-)preferred-params

@vemv
Copy link
Member

vemv commented May 7, 2024

I appreciate the receptiveness and patience 🙌

I'll probably be fine with any naming that you find suitable.

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 7, 2024

Oh and separately, we should note that there's an existing family of defcustoms named
cider-lein-parameters
cider-clojure-cli-parameters
[etc]

These are specifically related to a jack-in based workflow and have no effect on cider-connect. And despite cider-jack-in-* also taking a params plist as argument, they are actually strings appended in a tool-specific manner to the command line invocations.

I imagine a possible confusion could arise when a user wants to specify a fixed port while jacking in (eg. so she can dependably connect from another process which has no access to the generated .nrepl-port).
There's no tool-independent way of specifying this, so the names and docstrings for this new feature should make it clear that they don't affect jack-in commands.

Whether or not they should is a trickier matter (and probably out of scope for this PR), but might affect the naming if we decide to tackle it in the future.

@vemv
Copy link
Member

vemv commented May 7, 2024

There's no tool-independent way of specifying this, so the names and docstrings for this new feature should make it clear that they don't affect jack-in commands.

SGTM, so indeed making -connect part of the naming would seem a good idea.

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 28, 2024

I've rebased onto master and pushed a change for cider-connect-clj&cljs that I'm not entirely sure about, would appreciate someone else testing it in practice :)

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 28, 2024

Another note: I discovered this other variable cider-known-endpoints after making these changes. It lets you add a custom global entry to the "Select host" dialog (skipping one menu instead of two), which overlaps somewhat with the intention of this PR.. maybe a better alternative would be to extend the existing functionality and improve its discoverability via the online documentation?

@vemv
Copy link
Member

vemv commented May 28, 2024

Thanks for pushing through this!

cider-known-endpoints looks pretty limited as it is right now, as it's unaware of project-dir and language.

Regardless yes, it could plausibly be extended to satisfy current needs.

I'd say, feel free to explore in that direction. If it's too hard to keep using it with extended semantics in a non-breaking manner, we could also favor this new PR.

@vemv
Copy link
Member

vemv commented May 28, 2024

You wrote in Slack:

Yeah, the docstring is a bit confusing and it affects the host selection dialog on a global basis even if you set it in another project's dir-locals.el

Perhaps the 'globality' is enough of a reason to deprecate the var. Most likely changing the var type would be a breaking change.

@bbatsov
Copy link
Member

bbatsov commented May 29, 2024

I'd say, feel free to explore in that direction. If it's too hard to keep using it with extended semantics in a non-breaking manner, we could also favor this new PR.

Agreed. I guess we had mostly forgotten about it, but it should be hard to add the newer params to it.

Yeah, the docstring is a bit confusing and it affects the host selection dialog on a global basis even if you set it in another project's dir-locals.el

It wasn't meant to be used in .dir-locals.el.

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