Recompute namespace info on each shadow-cljs recompilation or evaluation #3396
Conversation
|
Generally, it's good to mention all hooks in the user docs as well. |
cider-repl.el
Outdated
| (when (nrepl-op-supported-p "cider/get-state" conn) | ||
| (nrepl-send-request '("op" "cider/get-state") nil conn)))))) | ||
|
|
||
| (defun cider--shadow-cljs-stderr-hook (buffer err) |
There was a problem hiding this comment.
The list of functions to call is called a "hook", not the functions in it.
cider-repl.el
Outdated
| "Refresh the changed namespaces metadata given BUFFER and ERR." | ||
| (cider--maybe-get-state-for-shadow-cljs buffer err)) | ||
|
|
||
| (defun cider--shadow-cljs-done-hook (buffer) |
cider-repl.el
Outdated
| "Refresh the changed namespaces metadata given BUFFER." | ||
| (cider--maybe-get-state-for-shadow-cljs buffer)) | ||
|
|
||
| (defcustom cider-repl-stdout-hooks nil |
There was a problem hiding this comment.
hooks -> hook
Refer to other hooks in the codebase.
cider-repl.el
Outdated
| @@ -994,14 +1044,20 @@ nREPL ops, it may be convenient to prevent inserting a prompt.") | |||
| (lambda (buffer value) | |||
| (cider-repl-emit-result buffer value t)) | |||
| (lambda (buffer out) | |||
| (dolist (f cider-repl-stdout-hooks) | |||
There was a problem hiding this comment.
You should see if you can use run-hooks here. I don't recall how it played for hooks with arguments.
There was a problem hiding this comment.
run-hooks docstring says that no arguments will be passed.
In face of that, maybe we can refrain from using the hook terminology here?
e.g. defcustom cider-repl-stdout-callbacks/ defcustom cider-repl-stdout-functions.
(callbacks would seem more expressive to me here)
There was a problem hiding this comment.
It depends on whether you expect users to actually do something here or not - if you do I'd stick with hooks, just because it's the established term in the community. If not you can just makes so defvars (perhaps private) and then the names won't matter as much.
There was a problem hiding this comment.
I'd be wary of creating 'hooks' that couldn't be run with run-hooks - could be confusing and imcompatible with other hook-related features.
Defvars make sense :) we can always promote them later.
|
Ready, QAed again as well. |
| @@ -996,12 +1034,16 @@ nREPL ops, it may be convenient to prevent inserting a prompt.") | |||
| (lambda (buffer out) | |||
| (cider-repl-emit-stdout buffer out)) | |||
| (lambda (buffer err) | |||
| (dolist (f cider--repl-stderr-functions) | |||
There was a problem hiding this comment.
There's one more thing to consider here (for output in general and for values) - as this can be triggered a ton of times for something generating chunked output it can degrade performance significantly.
There was a problem hiding this comment.
Note that the guards are carefully ordered:
(when (and (eq cider-repl-type 'cljs)
(eq cider-cljs-repl-type 'shadow)
(not cider-repl-cljs-upgrade-pending)
(if err
(string-match-p "Build completed\\." err)
t))no matter how large the output we're receiving is, for most cases we're not even inspecting it.
WIth that said, yesterday I created thheller/shadow-cljs#1139 which is going in a good direction. A few features could be built on top of it, and then this could be discarded.
There was a problem hiding this comment.
Yeah, yeah - I'm aware of this. Just a point of concern, as we have to be careful about this. Hopefully we'll come up with a better solution down the road.
|
Looks good. Just rebase and squash before merging. |
Similar to #3396, except that it's only necessary on recompilation - not on evaluations.
Similar to #3396, except that it's only necessary on recompilation - not on evaluations.
Similar to #3396, except that it's only necessary on recompilation - not on evaluations.
Similar to #3396, except that it's only necessary on recompilation - not on evaluations.
Similar to #3396, except that it's only necessary on recompilation - not on evaluations.
Fixes #3393
Works thanks to clojure-emacs/cider-nrepl#799
I tested this one locally.
The defcustoms are bit of a POC - feedback welcome. Initially I directly inlined the code, later I figured that it would pollute the main logic.
Cheers - V