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.

Sign up for GitHub

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

Jump to bottom

Preserve the :cljs-repl-type more reliably #3378

Merged
merged 4 commits into from
Jul 26, 2023
Merged

Preserve the :cljs-repl-type more reliably #3378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dcebfde
Preserve :cljs-repl-type when specified
vemv Jul 22, 2023
9411aa9
Linter
vemv Jul 22, 2023
04dbe2e
Remove now-duplicated code
vemv Jul 22, 2023
d949671
Add comment
vemv Jul 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

- Bump the injected `cider-nrepl` to [0.32](https://github.com/clojure-emacs/cider-nrepl/blob/master/CHANGELOG.md).
- Improve `nrepl-dict` error reporting.
- Preserve the `:cljs-repl-type` more reliably.

## 1.7.0 (2023-03-23)

Expand Down
4 changes: 3 additions & 1 deletion cider-connection.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -788,7 +788,7 @@ Session name can be customized with `cider-session-name-template'."
;;; REPL Buffer Init

(defvar-local cider-cljs-repl-type nil
"The type of the ClojureScript runtime (Browser, Node, Figwheel, etc.).")
"The type of the ClojureScript runtime ('browser, 'node, 'figwheel, etc.).")

(defvar-local cider-repl-type nil
"The type of this REPL buffer, usually either clj or cljs.")
Expand Down Expand Up @@ -919,6 +919,8 @@ function with the repl buffer set as current."
;; ran at the end of cider--connected-handler
cider-repl-init-function (plist-get params :repl-init-function)
cider-launch-params params)
(when-let ((type (plist-get params :cljs-repl-type)))
(setq cider-cljs-repl-type type))
Comment on lines +922 to +923
Copy link
Member Author

Choose a reason for hiding this comment

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

The main fix

There's an (setq-local cider-cljs-repl-type cljs-type) elsewhere but it didn't have effect (or was flaky) for me.

Copy link
Member

Choose a reason for hiding this comment

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

Where it exactly? Shouldn't we remove it if seems it's not working properly?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's in cider--update-cljs-init-function

My tentative thinking was to leave it there. The worst thing that could happen is that it's set the same value twice.

It's fairly hard to test all code paths (given the many cljs runtimes) so it can be considered cautious (at the cost of duplication).

I can remove it though if I see that things indeed keep work locally.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the duplicated code and verified that it still works.

Which is more than reasonable. The new (setq cider-cljs-repl-type type) is performed as soon as the repl buffer is created, so it's unlikely that the cljs-repl-type will suddenly change (especially given the recent #3376).

(cider-repl-reset-markers)
(add-hook 'nrepl-response-handler-functions #'cider-repl--state-handler nil 'local)
(add-hook 'nrepl-connected-hook #'cider--connected-handler nil 'local)
Expand Down
29 changes: 19 additions & 10 deletions cider.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1293,7 +1293,9 @@ double prefix prompt for all these parameters."
(defun cider-jack-in-cljs (params)
"Start an nREPL server for the current project and connect to it.
PARAMS is a plist optionally containing :project-dir, :jack-in-cmd and
:cljs-repl-type (e.g. Node, Figwheel, etc). With the prefix argument,
:cljs-repl-type (e.g. 'shadow, 'node, 'figwheel, etc).

With the prefix argument,
allow editing of the jack in command; with a double prefix prompt for all
these parameters."
(interactive "P")
Expand All @@ -1318,9 +1320,12 @@ these parameters."
(defun cider-jack-in-clj&cljs (&optional params soft-cljs-start)
"Start an nREPL server and connect with clj and cljs REPLs.
PARAMS is a plist optionally containing :project-dir, :jack-in-cmd and
:cljs-repl-type (e.g. Node, Figwheel, etc). With the prefix argument,
allow for editing of the jack in command; with a double prefix prompt for
all these parameters. When SOFT-CLJS-START is non-nil, start cljs REPL
:cljs-repl-type (e.g. 'shadow, 'node, 'fighweel, etc).

With the prefix argument, allow for editing of the jack in command;
with a double prefix prompt for all these parameters.

When SOFT-CLJS-START is non-nil, start cljs REPL
only when the ClojureScript dependencies are met."
(interactive "P")
(let ((cider-jack-in-dependencies (append cider-jack-in-dependencies cider-jack-in-cljs-dependencies))
Expand Down Expand Up @@ -1371,13 +1376,17 @@ server is created."
;;;###autoload
(defun cider-connect-sibling-cljs (params &optional other-repl)
"Create a ClojureScript REPL with the same server as OTHER-REPL.
PARAMS is a plist optionally containing :cljs-repl-type (e.g. Node,
Figwheel, etc). All other parameters are inferred from the OTHER-REPL.
PARAMS is a plist optionally containing :cljs-repl-type (e.g. 'node,
'figwheel, 'shadow, etc).

All other parameters are inferred from the OTHER-REPL.
OTHER-REPL defaults to `cider-current-repl' but in programs can also be a
server buffer, in which case a new session for that server is created."
(interactive "P")
(let* ((other-repl (or other-repl (cider-current-repl 'any 'ensure)))
(other-params (cider--gather-connect-params nil other-repl))
;; type-related params from the JVM conn are undesired for a cljs conn:
(other-params (thread-first other-params (map-delete :repl-type) (map-delete :cljs-repl-type)))
Copy link
Member Author

@vemv vemv Jul 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For a cljs connection, the jvm repl-type and cljs-repl-type are irrelevant / can cause conflicts

I was seeing plist values such as (:cljs-repl-type :cljs ... :cljs-repl-type nil) i.e. with duplication coming from other-params

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 add a comment here explaining this for posterity's sake.

(ses-name (unless (nrepl-server-p other-repl)
(sesman-session-name-for-object 'CIDER other-repl))))
(cider-nrepl-connect
Expand Down Expand Up @@ -1410,7 +1419,9 @@ prefix argument, prompt for all the parameters."
(defun cider-connect-cljs (&optional params)
"Initialize a ClojureScript connection to an nREPL server.
PARAMS is a plist optionally containing :host, :port, :project-dir and
:cljs-repl-type (e.g. Node, Figwheel, etc). On prefix, prompt for all the
:cljs-repl-type (e.g. 'shadow, 'node, 'figwheel, etc).

On prefix, prompt for all the
parameters regardless of their supplied or default values."
(interactive "P")
(cider-nrepl-connect
Expand All @@ -1428,7 +1439,7 @@ parameters regardless of their supplied or default values."
(defun cider-connect-clj&cljs (params &optional soft-cljs-start)
"Initialize a Clojure and ClojureScript connection to an nREPL server.
PARAMS is a plist optionally containing :host, :port, :project-dir and
:cljs-repl-type (e.g. Node, Figwheel, etc). When SOFT-CLJS-START is
:cljs-repl-type (e.g. 'shadow, 'node, 'figwheel, etc). When SOFT-CLJS-START is
non-nil, don't start if ClojureScript requirements are not met."
(interactive "P")
(let* ((params (thread-first
Expand Down Expand Up @@ -1630,8 +1641,6 @@ over to cljs.
(plist-put :repl-init-function
(lambda ()
(cider--check-cljs cljs-type)
;; FIXME: ideally this should be done in the state handler
(setq-local cider-cljs-repl-type cljs-type)
(cider-nrepl-send-request
(list "op" "eval"
"ns" (cider-current-ns)
Expand Down
Loading