tpope / vim-fireplace

fireplace.vim: Clojure REPL support
https://www.vim.org/scripts/script.php?script_id=4978
1.74k stars 139 forks source link

Add wrapper function around :Piggieback for Figwheel.main #326

Closed Jens-H closed 5 years ago

Jens-H commented 5 years ago

A lot of repeating text is necessary, to connect to the cljs repl of the new Figwheel.main via the :Piggieback function. I added a wrapper function :Fig that removes the need to type all this unnecessary text.

Before: :Piggieback (figwheel.main.api/repl-env "dev")

Now: :Fig dev

Maybe this is something to share with everyone.

Deraen commented 5 years ago

With #328 this might be less useful. Figwheel users could set let g:fireplace_cljs_repl_opts = {'ns': 'figwheel.main.api', 'fn': 'repl-env "dev"'} and I think Fireplace should connect to Figwheel automatically when trying to evaluate Cljs code.

Jens-H commented 5 years ago

Indeed this looks like a more complete solution.

However, I found a problem calling figwheel.main with piggieback (see https://github.com/bhauman/figwheel-main/issues/115). Basically I had to call (figwheel.main.api/cljs-repl "dev") without piggieback.

If this is the intended behavior from figwheel, then we need more adjustments.

Jens-H commented 5 years ago

The before mentioned bug in figwheel-main should be solved with the next release. So my patch would be working again soon.

tpope commented 5 years ago

What is your workflow for using Figwheel with Fireplace? When and where do you call (figwheel.main.api/start "dev")?

Jens-H commented 5 years ago

I am pretty much following the docs from figwheel https://figwheel.org/docs/vim.html Though, the (n)repl server is started with a script using deps/cli. Also the steps to start figwheel (require, figwheel.main.api/start, figwheel.main.api/cljs-repl) are scripted, but essentially like in the docs. Of course there could be all kinds of options passed to the "figwheel.main.api/start"' function.

Afterwards I can open a cljs file in vim and run :Fig dev or :Piggieback (figwheel.main.api/repl-env "dev") - if the .nrepl-port file is available.

Btw., great to see all those enhancements in fireplace lately!

tpope commented 5 years ago

I've made :Piggieback (figwheel.main.api/cljs-repl "dev") now do the right thing and omit the cider.piggieback wrapper. (This means it should probably be renamed from :Piggieback to something more generic at some point.) let g:fireplace_cljs_repl = '(figwheel.main.api/cljs-repl "dev")' should now streamline the process significantly.

I am positively irked that Figwheel's start complains and bails if an environment is already running, and clj-repl complains and bails if an environment isn't already running. This is screaming for an idempotent function, but instead, I'm supposed to waste brain power on remembering whether I started the server or not and manually invoke the correct function.

What's the story with other Figwheel libraries like sidecar? If we do provide a command like :Figwheel, it'd be weird if it only supported Figwheel.main, no? (:Fig is just asinine, spell the damn thing out.)

Jens-H commented 5 years ago

I have tried the new :Piggieback call with the global var. This is working for me.

One thing that would be possible with the wrapper :Figwheel (your track record of giving names is for a good reason much larger) is to use other names than "dev" easily. Not sure if this justifies to introduce a new command. Especially considering it is only for Figwheel.main.

I don't use the older lein-figwheel/sidecar, therefore I am not sure what to do with it. Looks like they are quite a bit different. So maybe not combining them would be reasonable too.

tpope commented 5 years ago

Do you know if sidecar is older in a "not really used for new projects" sort of way, or a "serves a different but still important purpose" sort of way? I'm way out of the loop on this.

Jens-H commented 5 years ago

Honestly I don't have the experience to answer this. However, the website says:

The original lein-figwheel is still relevant and widely used.

https://figwheel.org/docs/#figwheel-main-vs-lein-figwheel

I would think, that we can defer the decision until there is more evidence about the need for an extra command. My fork is available for interested persons.

tpope commented 5 years ago

You can turn your fork into an external one liner with something like this:

autocmd FileType clojure command! -buffer -nargs=1 Figwheel Piggieback (figwheel.main.api/cljs-repl "<args>")
Jens-H commented 5 years ago

Just for the record: I think it should be autocmd FileType clojure command! -buffer -nargs=1 Figwheel Piggieback (figwheel.main.api/repl-env "<args>") using repl-env instead of cljs-repl. Both worked in my test, but clj-repl was my workaround for a problem in figwheel.

This approach seems to be the best. So I guess we can close this pull request. Thanks a lot for your help.

tpope commented 5 years ago

This issue?

I believe the problem is, that vim-fireplace's function :Piggieback is wrapping around some more Clojure code and in the end the result is something like this:

((or (resolve 'cider.piggieback/cljs-repl) (resolve 'cemerick.piggieback/cljs-repl)) (figwheel.main.api/repl-env "dev"))

That is no longer the case, and Figwheel's docs explicitly say it's an inferior experience to cljs-repl. (They also say repl-env is essentially a workaround for Fireplace, we should probably get that removed at some point.)

Jens-H commented 5 years ago

You are right! Thanks for the correction.