Support cider.clj-reload/reload ops

filipesilva commented 7 months ago

See https://github.com/clojure-emacs/cider-nrepl/pull/850 for more.

Notes:

this ns only had one trivial test, I didn't add more
listing shows existing linting errors, I made sure I wasn't adding more

Replace this placeholder text with a summary of the changes in your PR. The more detailed you are, the better.

Before submitting the PR make sure the following things have been done (and denote this by checking the relevant checkboxes):

[x] The commits are consistent with our contribution guidelines
[x] You've added tests (if possible) to cover your change(s)
[x] All tests are passing (eldev test)
[x] All code passes the linter (eldev lint) which is based on elisp-lint and includes
- byte-compilation, checkdoc, check-declare, packaging metadata, indentation, and trailing whitespace checks.
[x] You've updated the changelog (if adding/changing user-visible functionality)
[x] You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its manual extremely useful.

filipesilva commented 7 months ago

@vemv just to get the ball rolling, is this roughly what you had in mind?

filipesilva commented 7 months ago

Btw have you seen this eldev error? It happens on build and test:

[I] filipesilva@Filipes-MacBook-Pro ~/s/cider (clj-reload) [1]> eldev test
Native compiler error: (lambda (arg0 &rest arg1) (let ((f #'message)) (apply f arg0 arg1))), "Compiling /Users/filipesilva/.emacs.d/eln-cache/29.1-272591eb/subr--trampoline-6d657373616765_message_0.eln...
-macosx_version_min has been renamed to -macos_version_min
ld: library 'emutls_w' not found
libgccjit.so: error: error invoking gcc driver
Internal native compiler error: \"failed to compile\", \"/Users/filipesilva/.emacs.d/eln-cache/29.1-272591eb/subr--trampoline-6d657373616765_message_0.eln\", \"error invoking gcc driver\"

Error: native-ice (\"failed to compile\" \"/Users/filipesilva/.emacs.d/eln-cache/29.1-272591eb/subr--trampoline-6d657373616765_message_0.eln\" \"error invoking gcc driver\")
  mapbacktrace(#f(compiled-function (evald func args flags) #<bytecode -0xc883f0734512f81>))
  debug-early-backtrace()
  debug-early(error (native-ice \"failed to compile\" \"/Users/filipesilva/.emacs.d/eln-cache/29.1-272591eb/subr--trampoline-6d657373616765_message_0.eln\" \"error invoking gcc driver\"))
  comp--compile-ctxt-to-file(\"/Users/filipesilva/.emacs.d/eln-cache/29.1-272591eb/subr--trampoline-6d657373616765_message_0.eln\")
  comp-compile-ctxt-to-file(\"/Users/filipesilva/.emacs.d/eln-cache/29.1-272591eb/subr--trampoline-6d657373616765_message_0.eln\")
  comp-final1()
  load-with-code-conversion(\"/private/var/folders/hx/dl3d0pqn6615z78g83gc26300000gn/T/emacs-int-comp-subr--trampoline-6d657373616765_message_0-re9NpE.el\" \"/private/var/folders/hx/dl3d0pqn6615z78g83gc26300000gn/T/emacs-int-comp-subr--trampoline-6d657373616765_message_0-re9NpE.el\" nil t)
  command-line-1((\"-l\" \"/var/folders/hx/dl3d0pqn6615z78g83gc26300000gn/T/emacs-int-comp-subr--trampoline-6d657373616765_message_0-re9NpE.el\"))
  command-line()
  normal-top-level()
"

filipesilva commented 7 months ago

If anyone else comes across that error, reinstalling emacs from homebrew seemed to fix it.

filipesilva commented 7 months ago

@vemv was able to get a local dev env working, debugged the feature, and I believe this PR is ready to be reviewed. I left support for the clear operation, but will continue discussion of it in https://github.com/clojure-emacs/cider-nrepl/pull/850 instead.

vemv commented 7 months ago

@filipesilva : following https://github.com/clojure-emacs/cider/pull/3625 you may rebase master in to get cider-nrepl with your changes in 🎉

vemv commented 7 months ago

...and following https://github.com/clojure-emacs/cider/pull/3627 you should should see jumping to the culprit file/line on errors.

(No backend changes were needed at all)

Please let us know if it works here as well.

filipesilva commented 7 months ago

@vemv @bbatsov I've pushed changes that address your review items. The review items that were trivial to incorporate I marked as resolved to better keep track of things.

vemv commented 7 months ago

@filipesilva : a very common .dir-locals.el is some variation of:

((nil . ((cider-ns-refresh-before-fn . "integrant.repl/suspend")
         (cider-ns-refresh-after-fn  . "integrant.repl/resume")
,,,

So that reloaded changes can actually affect an app.

I'd be OK with merging this asap but please confirm if you'd intend to further refine the solution

filipesilva commented 7 months ago

@vemv I'm not really thinking of adding that extra functionality, no. That looks like it should be part of base functionality for clj-reload, not something that needs CIDER to work. It might already work just with unload hooks, but I'm not sure. @tonsky what are your thoughts on this?

filipesilva commented 7 months ago

Meanwhile I found a bug where reload-all isn't working because I passed the wrong option in cider-nrepl. I'm working on fixing it and making the tests more robust.

filipesilva commented 7 months ago

https://github.com/clojure-emacs/cider-nrepl/pull/854 is the fix for the reload-all op.

tonsky commented 7 months ago

@vemv I'm not really thinking of adding that extra functionality, no. That looks like it should be part of base functionality for clj-reload, not something that needs CIDER to work. It might already work just with unload hooks, but I'm not sure. @tonsky what are your thoughts on this?

I think this is just calling a function immediately before clj-reload.core/reload and immediately after. What kind of support are you thinking? Can clj-reload really do something here?

vemv commented 7 months ago

Thanks!

While t.n supports an :after fn, in cider-nrepl, (somewhat surprisingly) we invoke the functions ourselves.

So yeah no upstream changes are required.

vemv commented 6 months ago

I'm squash-merging this separately as we speak.

Thanks much for your work @filipesilva !

vemv commented 6 months ago

It's in master now

https://github.com/clojure-emacs/cider/commit/c4fa1a84a3b3d03ef5f61cc5d33ff4e91b9a1dce

A melpa snapshot will be automatically created later today.

We'll cut a stable release soon enough.

Cheers - V

filipesilva commented 6 months ago

Awesome, thank you so much!

filipesilva commented 6 months ago

Just following up on the "reloaded changes can actually affect an app" case.

I can verify I can add this fn in a given ns:

(defn before-ns-unload []
  (println "stopping processes")

(defn after-ns-reload []
  (println "restarting processes")

And it is running before/after the reload, both when I call reload manually and when I call it via cider. I just used it to restart a server in an app of mine I've moved to clj-reload. So clj-reload can handle this use case via its hooks.

vemv commented 6 months ago

Thanks!

Anyway, cider-ns-refresh-before-fn / cider-ns-refresh-after-fn are now supported, so it should work as well.

cichli commented 4 months ago

While t.n supports an :after fn, in cider-nrepl, (somewhat surprisingly) we invoke the functions ourselves.

The middleware interleaves responses such as {:status :ok} / {:status :invoking-after} / {:status :invoked-after} with the execution of the refresh and invoking :after – if we used the t.n option, we wouldn't be able to do that.

clojure-emacs / cider

Support cider.clj-reload/reload ops #3624