tpope / vim-fireplace

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

Cannot reload file with shadow-cljs nREPL connection #385

Closed apirogov closed 3 years ago

apirogov commented 3 years ago

I created a re-frame project with lein new re-frame reframe-test and am trying to setup Fireplace with shadow-cljs and followed instructions like e.g. here, have added cider-nrepl as dependency, etc.

It seems to connect just fine and using cqp to enter the quasi-shell and evaluating some simple command works and I can also require modules, but:

For some reason I cannot use cpr to reload a file as I would usually do with Clojure. I get the following error:

[2021-05-08 14:39:24.512 - WARNING] :shadow.cljs.devtools.server.worker.impl/cljs-compile-ex - {:input {:code "(load-file \"reframe_test/core.cljs\")", :ns cljs.user, :repl true}}
ExceptionInfo Failed to process REPL command {:eof? false, :ns cljs.user, :form (load-file "reframe_test/core.cljs"), :source "(load-file \"reframe_test/core.cljs\")", :tag :shadow.cljs.repl/process-ex}
    shadow.cljs.repl/process-read-result (repl.clj:523)
    shadow.cljs.repl/process-read-result (repl.clj:497)
    shadow.cljs.repl/process-input (repl.clj:681)
    shadow.cljs.repl/process-input (repl.clj:659)
    shadow.cljs.devtools.server.worker.impl/fn--14405 (impl.clj:754)
    shadow.cljs.devtools.server.worker.impl/fn--14405 (impl.clj:744)
    clojure.lang.MultiFn.invoke (MultiFn.java:234)
    shadow.cljs.devtools.server.util/server-thread/fn--14068/fn--14069/fn--14077 (util.clj:284)
    shadow.cljs.devtools.server.util/server-thread/fn--14068/fn--14069 (util.clj:283)
    shadow.cljs.devtools.server.util/server-thread/fn--14068 (util.clj:256)
    java.lang.Thread.run (Thread.java:829)
Caused by:
ExceptionInfo file not on classpath {:file-path "reframe_test/core.cljs"}
    shadow.cljs.repl/repl-load-file* (repl.clj:294)
    shadow.cljs.repl/repl-load-file* (repl.clj:282)
    shadow.cljs.repl/repl-load-file (repl.clj:330)
    shadow.cljs.repl/repl-load-file (repl.clj:328)
    shadow.cljs.repl/process-read-result (repl.clj:517)
    shadow.cljs.repl/process-read-result (repl.clj:497)

I have not touched any paths and essentially just try to connect to the default template stub, the shadow-cljs.edn includes the src directory which contains the "reframe_test/core.cljs", so I wonder if I am doing something wrong or this is a bug...

I use cider-nrepl 0.26.0 (I also tried with 0.22.4 with the same problem) and it says shadow-cljs - server version: 2.12.5. I tried with a minimal vim (Version 8.2) config that basically just loads fireplace, to exclude the possibility that other plugins mess up paths or something. But if it is the path, I wonder why require in the quasi-REPL works.

tpope commented 3 years ago

I would start by double checking in a REPL that the classpath is what you expect it to be.

apirogov commented 3 years ago

Running npx shadow-cljs clj-repl and (System/getProperty "java.class.path") returns a huge classpath that begins with src:... and dozens of lines with dependency jars. Also, changing things in a cljs file leads to automatic reload and update in the browser. So I guess it is fine...

EDIT: I tried doing load-file in the cljs console by hand, and with a prefixed src/ it works. I have no idea whether the shadow-cljs version of load-file has some bug and it's actually supposed to look in all java classpath directories (then that bug must be old, as I went to versions from 2019 and had the same behaviour), but without src/ I get the "not on classpath" error.

tpope commented 3 years ago

Check :echo fireplace#cljs().Path() to see if the path makes it over correctly to Fireplace. This probably isn't the issue but let's rule it out as a possible factor.

apirogov commented 3 years ago

It seems to work fine. Returns a huge array, where the paths are expanded to absolute paths. The first one is the absolute path of the src folder.

tpope commented 3 years ago

Now that I look at it, I'm not sure what I was thinking in 226c5a0fd114cab535871185897bec5558e41297. I assume require wasn't working, but load-file doesn't use the class path at all, so it isn't a good drop in replacement for it. You said require works for you? If so, I think we should just remove the special case and use require everywhere.

apirogov commented 3 years ago

Ok, I am rather new to Clojure and especially ClojureScript so I don't know whether there a subtleties in this... Like e.g., require working with shadow-cljs but not figwheel or something like this. At least with a shadow-cljs setup, apparently using require returns nil and does not throw a stacktrace, so I assume that it works. So maybe the workaround is not needed (anymore), but if it is, then maybe the load-file can be just fixed by providing the absolute path, instead of relative as seen in the stacktrace.

But I just noticed that maybe cpr is not so useful for CLJS, as saving a file triggers automatic recompilation and update of the view, so maybe that's the reason that no one ever complained about this before.

tpope commented 3 years ago

One more thing, can you check if (require ... :reload) works? Since that's what will actually be called.

Ok, I am rather new to Clojure and especially ClojureScript so I don't know whether there a subtleties in this... Like e.g., require working with shadow-cljs but not figwheel or something like this. At least with a shadow-cljs setup, apparently using require returns nil and does not throw a stacktrace, so I assume that it works. So maybe the workaround is not needed (anymore), but if it is, then maybe the load-file can be just fixed by providing the absolute path, instead of relative as seen in the stacktrace.

I almost proposed this, but since the command is called :Require, we should probably use require unless there's a really good reason not to. I don't trust that 2014 me had a good reason.

But I just noticed that maybe cpr is not so useful for CLJS, as saving a file triggers automatic recompilation and update of the view, so maybe that's the reason that no one ever complained about this before.

I think you're right.

apirogov commented 3 years ago

One more thing, can you check if (require ... :reload) works? Since that's what will actually be called.

(require ...), (require ... :reload) and (require ... :reload-all) all seem to work without any complaints.

tpope commented 3 years ago

Oh and can you remind me if the fully qualified form is (cljs.core/require) or (clojure.core/require)?

tpope commented 3 years ago

Scratch that, I'll use (ns cljs.user (:require ... :reload)) style which has been vetted by our preload(). Can you test the cljs-require branch and make sure it works? This will save me the hour or more it will take me to remember how to boot a CLJS app.

apirogov commented 3 years ago

It works! But please make it a "silent" command (so I don't have to see the command or press anything afterwards).

tpope commented 3 years ago

We can't kill the output without killing any error messages, but I suppose getting rid of the echoed command wouldn't be the end of the world.