search

day8 / re-frame

A ClojureScript framework for building user interfaces, leveraging React
http://day8.github.io/re-frame/
MIT License
5.42k stars 715 forks source link

Incorrect docstring for `re-frame.core/debug` #676

Closed p-himik closed 1 year ago

p-himik commented 3 years ago

The docstring says:

An interceptor which logs/instruments an event handler's actions to
  `js/console.debug`

The actual implementation in re-frame.std-interceptors uses:

(console :log "Handling re-frame event:" (get-coeffect context :event))
[...]
(console :log "only before:" only-before)
(console :log "only after :" only-after)

Apart from using an incorrect level, the docstring also mentions js/console, which is incorrect.

I'd argue that both places should use/mention re-frame.core/console. And definitely use/mention the same logging level, but whether :log or :debug I'm not sure.

jimpil commented 1 year ago

I would also add the fact that the diff parts are console-printed completely bare - i.e. as Objects. That is not very helpful, unless you have extra tooling in place... I would have expected everything to to be printed out as EDN, otherwise what's the point? How am I expected to make sense of these Objects? Is no-one else having this problem?

p-himik commented 1 year ago

@jimpil Do you have cljs-devtools installed?

jimpil commented 1 year ago

That's the extra tooling I was referring to - I guess I can install it, but I kind of feel I shouldn't have to...

p-himik commented 1 year ago

In an ideal world, CLJS runs directly in a browser. ;) But cljs-devtools is a must-have tool - same as re-frame-10x, React DevTools, shadow-cljs build report and so on.

Printing the EDN itself as a string would be much less useful - no interaction, potential data size issues, no proper representation for types that EDN doesn't handle, inability to drill down (e.g. you can right-click an expanded object when you have cljs-devtools and save it in the global scope under its own name).

mike-thompson-day8 commented 1 year ago

@jimpil I second the previous comment. cljs-devtools is essential when doing anything in clojurescript.

mike-thompson-day8 commented 1 year ago

Two fixes are possible:

  1. Change the interceptor to use (console :debug ...) instead of (console :log ...)
  2. Change the comment so it correctly says how the output is consoled (js/console.log not js/console.debug)

I note that approach 1 is a slightly breaking change.

Preferences?

Either way, the docstring should be adjusted to mention that it uses re-frame.core/console rather than js/console

kimo-k commented 1 year ago

Going with fix number 2 for now. @mike-thompson-day8