-
-
Notifications
You must be signed in to change notification settings - Fork 645
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
Preserve the :cljs-repl-type
more reliably
#3378
Conversation
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)) | ||
(other-params (thread-first other-params (map-delete :repl-type) (map-delete :cljs-repl-type))) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
(when-let ((type (plist-get params :cljs-repl-type))) | ||
(setq cider-cljs-repl-type type)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
The
:cljs-repl-type
was easily lost. So I would see in a repl buffer name the":shadow"
suffix intermittently.A couple fixes in the PR help preserve it better.
I verified locally that it works.