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

Regression: can no longer specify custom cljs REPL code via variable #2572

Closed
weavejester opened this issue Jan 22, 2019 · 9 comments
Closed
Labels
enhancement good first issue A simple tasks suitable for first-time contributors pinned Tickets immune to staleness checks

Comments

@weavejester
Copy link

Older versions of CIDER had a cider-cljs-lein-repl variable, but this appears to be deprecated in favour of REPL types, leading to a regression in functionality.

If we want to specify our own code to initiate a cljs REPL, then we have to use cider-register-cljs-repl-type. However, this is not easily useable from .dir-locals.el. Ideally we want to be able to allow projects to specify their own cljs setup, as was the case in earlier versions of CIDER.

A possible solution is to add a cider-cljs-custom-repl variable, which when set would be used instead of the REPL types.

On Slack "ag" pointed me toward a hacky workaround using eval:

((nil . ((eval . (with-eval-after-load 'cider
                   (cider-register-cljs-repl-type
                    'fw-cc
                    "(do (start) (start-fw) (use 'figwheel-sidecar.repl-api) (cljs-repl))")
                   (setq cider-default-cljs-repl 'fw-cc))))))

However, that's not exactly elegant!

While I appreciate the need to improve features and streamline functionality, it's a touch frustrating to grab a new version of CIDER and find out that it breaks my workflow because functionality has been removed.

@bbatsov
Copy link
Member

bbatsov commented Jan 22, 2019

The registration function is meant to be used in your Emacs config, so in .dir-locals you’d just have to set one var as before. The old variable was removed simply because the build tool used and the cljs repl used are completely orthogonal. I’m sorry that this caused some frustration for you, but I think there was no easy way to retain the old behavior and provide the desired behavior at the same time.

@weavejester
Copy link
Author

The registration function is meant to be used in your Emacs config, so in .dir-locals you’d just have to set one var as before.

What variable would I set in .dir-locals.el? If I've missed a variable I apologize, but from my reading of the source there didn't seem to be a way to set a custom initialization string, except by triggering an eval and adding a new cljs REPL type.

The old way of doing it would be to add something like this to .dir-locals.el.

((nil . ((cider-cljs-lein-repl . "(do (custom-init) (cljs-repl))"))))

But since cider-cljs-lein-repl has been deprecated, is there a variable that we should be using instead that does the same thing?

@bbatsov
Copy link
Member

bbatsov commented Jan 23, 2019

My point was that REPL types should normally be registered globally - e.g. in init.el. You’d just refer to the REPL type in .dir-locals.el, you won’t be registering it there. The basic premise is that’s it’s very unlikely that someone would use REPL init forms that are unique to a single project, so it makes sense to register those globally. Does this make sense? I don’t have a computer with me and I can’t write detailed examples.

Currently there’s no way to do exactly what cider-cljs-lein-repl did, but we can provide something if it’s warranted.

@weavejester
Copy link
Author

weavejester commented Jan 23, 2019

The basic premise is that’s it’s very unlikely that someone would use REPL init forms that are unique to a single project, so it makes sense to register those globally.

I agree it's not likely that a REPL init form will be limited to a single project; however, it is likely that there will be projects where the required REPL init form is not included in CIDER by default. For example, any project that uses Duct (hence my interest!).

Before, we could include a .dir-locals.el file in the directory, and anyone using Emacs and CIDER who opens the project will automatically be able to load in a ClojureScript REPL. With the current change to the REPL init form, I have to manually tell users how to set it up, with code to copy and paste into their init.el. If anything goes awry, they'll raise an issue.

So hopefully you can see why I'm so keen on a solution that can fit in a .dir-locals.el - because it provides per-project, automatic configuration.

@kommen
Copy link
Contributor

kommen commented Feb 27, 2019

The basic premise is that’s it’s very unlikely that someone would use REPL init forms that are unique to a single project, so it makes sense to register those globally.

Just adding a data point: we need this too.

Our solution right now is that our wrapper bash script, which starts the project's clojure app in development, uses emacsclient to add our custom cljs repl type to cider-cljs-repl-types. Not pretty, but it does the job.

@bbatsov
Copy link
Member

bbatsov commented Apr 2, 2019

I guess a good option might be to add something called cider-cljs-repl-form (or whatever) that'd would point to a form and would take precedence over the default type. We can also change cider-cljs-default-repl to be behave differently if it's a symbol or a string.

Basically, changing this is simple but we need to decide on the best "API". :-)

@bbatsov bbatsov added enhancement good first issue A simple tasks suitable for first-time contributors labels Apr 2, 2019
@stale
Copy link

stale bot commented May 8, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

@stale stale bot added the stale label May 8, 2019
@bbatsov bbatsov added the pinned Tickets immune to staleness checks label May 8, 2019
@stale stale bot removed the stale label May 8, 2019
@bbatsov
Copy link
Member

bbatsov commented Jun 16, 2019

After some consideration I've decided the best approach for this would be adding 'init-form as a cider-default-cljs-repl type and cider-cljs-repl-init-form variable that goes along with it. This fits nice within the current approach and will solve cleanly the problem in question. Expect a fix soon. This comment is mostly a reminder for myself, but everyone's welcome to beat me to implementing this simple fix.

@bbatsov
Copy link
Member

bbatsov commented Sep 23, 2019

In the end I've decided to just recycle the existing custom repl type:

((nil
  (cider-custom-cljs-repl-init-form . "(do (foo) (bar))"
  (cider-default-cljs-repl . custom)))

I think you'll all agree that's simple enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good first issue A simple tasks suitable for first-time contributors pinned Tickets immune to staleness checks
Projects
None yet
Development

No branches or pull requests

3 participants