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

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

Closed
p-himik opened this issue Feb 8, 2021 · 7 comments
Closed

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

p-himik opened this issue Feb 8, 2021 · 7 comments
Labels

Comments

@p-himik
Copy link
Contributor

p-himik commented Feb 8, 2021

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
Copy link

jimpil commented Jun 15, 2023

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
Copy link
Contributor Author

p-himik commented Jun 15, 2023

@jimpil Do you have cljs-devtools installed?

@jimpil
Copy link

jimpil commented Jun 15, 2023

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
Copy link
Contributor Author

p-himik commented Jun 15, 2023

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
Copy link
Contributor

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

@mike-thompson-day8
Copy link
Contributor

mike-thompson-day8 commented Jun 16, 2023

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 added a commit that referenced this issue Jun 16, 2023
This interceptor actually uses `(re-frame/console :log)` which uses
js/console.log on javascript.

Corrected.

#676
kimo-k added a commit that referenced this issue Jun 16, 2023
The todomvc example doesn't use this interceptor.

Corrected.

#676
@kimo-k kimo-k mentioned this issue Jun 16, 2023
kimo-k added a commit that referenced this issue Jun 16, 2023
This interceptor actually uses `(re-frame/console :log)`, which goes
to js/console.log on javascript.

Corrected.

#676
kimo-k added a commit that referenced this issue Jun 16, 2023
The todomvc example doesn't use this interceptor.

Corrected.

#676
@kimo-k
Copy link
Contributor

kimo-k commented Jun 16, 2023

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

kimo-k added a commit that referenced this issue Jun 16, 2023
This interceptor actually uses `(re-frame/console :log)`, which goes
to js/console.log on javascript.

Corrected.

#676
kimo-k added a commit that referenced this issue Jun 16, 2023
The todomvc example doesn't use this interceptor.

Corrected.

#676
@kimo-k kimo-k closed this as completed Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants