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

ClojureScript dynamic font lock not working for new functions (var) #3393

Closed
nikolavojicic opened this issue Aug 5, 2023 · 5 comments · Fixed by #3396
Closed

ClojureScript dynamic font lock not working for new functions (var) #3393

nikolavojicic opened this issue Aug 5, 2023 · 5 comments · Fixed by #3396

Comments

@nikolavojicic
Copy link

First go through this issue #3385 and use cider-nrepl version 0.34.0.

Expected behavior

User defined functions should be dynamically highlighted.

Actual behavior

In ClojureScript, new user defined functions are not dynamically highlighted but old ones are (there is no such problem in Clojure).

258242216-97a9316e-a64a-4efd-881e-99b61a3842c4

Steps to reproduce the problem

  1. (setq cider-font-lock-dynamically '(macro core function var)) in your init
  2. Run cider-jack-in-cljs
  3. Choose shadow
  4. Choose :app
  5. Visit application in browser
  6. Write the code from above
  7. Evaluate that code

There is no such problem if you write (setq cider-font-lock-dynamically '(macro core function deprecated)) in your config, i.e. the problem is probably with var.

@vemv
Copy link
Member

vemv commented Aug 5, 2023

Thanks for the new issue, much easier to manage things that way 👍

@vemv vemv changed the title ClojureScript dynamic font lock not working for new functions ClojureScript dynamic font lock not working for new functions (var) Aug 6, 2023
@vemv
Copy link
Member

vemv commented Aug 6, 2023

I can repro (note to self: the key to reproing is creating a new function).

A few observations:

  • from the nrepl log, I can see that the changed-namespaces key sometimes doesn't come for cljs
    • e.g. for a jvm repl, it always comes after a simple repl interaction (eval op), for cljs, never
    • a way to cause changed-namespaces to be included: change between .cljs namespaces, causing a in-ns ,,, eval

It's as if cider.nrepl.middleware.track-state sometimes failed to send anything at all, for cljs. Worth noting that it sends stuff async, with an agent. Maybe errors are being swallowed, although error handling seems ok.


Fixing this should be pretty important beyond the syntax highlighting, because the track-state middleware supports all type of functionality.

@vemv
Copy link
Member

vemv commented Aug 6, 2023

I have the impression that the track-state middleware runs just once for cljs, then never again.

Maybe it has to do with middleware order?

This is shadow's order:

https://github.com/thheller/shadow-cljs/blob/bb586a0fab475c13c6d090164c4122f2782308e3/src/main/shadow/cljs/devtools/server/nrepl.clj#L98-L105

...emphasis on nrepl.middleware.interruptible-eval/interruptible-eval being placed after cider.nrepl/cider-middleware - it might be usually the opposite?

@bbatsov
Copy link
Member

bbatsov commented Aug 8, 2023

e.g. for a jvm repl, it always comes after a simple repl interaction (eval op), for cljs, never

It's possible, like many things in CIDER it was designed for Clojure in the days where ClojureScript wasn't really a thing. I'm also wondering if it behaves in the same way with different ClojureScript REPLs given all the differences between them.

vemv added a commit to clojure-emacs/cider-nrepl that referenced this issue Aug 9, 2023
vemv added a commit to clojure-emacs/cider-nrepl that referenced this issue Aug 9, 2023
@vemv
Copy link
Member

vemv commented Aug 11, 2023

Fixed. You'd need to use cider-nrepl 0.35.0 and cider master.

Fixing this resulted in an overall improvement, beyond font-locking. So thanks for the heads up - don't hesitate to report more issues!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants