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

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

merged 4 commits into from
Jul 26, 2023

Conversation

vemv
Copy link
Member

@vemv vemv commented Jul 22, 2023

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.

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)))
Copy link
Member Author

@vemv vemv Jul 22, 2023

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.

Comment on lines +922 to +923
(when-let ((type (plist-get params :cljs-repl-type)))
(setq cider-cljs-repl-type type))
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).

@vemv vemv requested a review from bbatsov July 22, 2023 10:37
Copy link
Member

@bbatsov bbatsov left a 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.

@bbatsov bbatsov merged commit cc31f5b into master Jul 26, 2023
19 of 20 checks passed
@bbatsov bbatsov deleted the cljs-repl-type branch July 26, 2023 12:06
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.

2 participants