Add configurability to logging and other bits

[lorddoig]

There's a few things here, but the main thing is logging so feel free to cherry pick.

I added the logging stuff because the console was drowning in messages about overwritten re-frame handlers every time I changed a file; it's very lightweight as you can see. I removed the console grouping stuff, as all console calls between group() and groupEnd() are gobbled up, whether generated by re-frame or not. It's also not a standard and will blow up on, for example, IE versions older than 11.

[lorddoig]

Actually the arglists stuff in core shouldn't be in this PR at all. My bad.

[mike-thompson-day8]

@lorddoig Thankyou for this upgrade. It might be a week before I get to review this. But, yes, this is an area that needs to be improved.

[mike-thompson-day8]

@lorddoig Okay I'm back on this.

There appears to be 3 parts in this patch:

1. argslist stuff
2. functions for clearing handlers
3. logging changes

Regarding point 1 - you say to ignore.
Regarding point 3 - I'm going to handling logging a different way (complementary to your approach) - more soon.

So that leaves point 2. I'm open to that change, but want to be sure I understand your motivations. Are you using figwheel? Is it figwheel file reloads which cause the warnings about re-loaded handlers that you mention?

[lorddoig]

@mike-thompson-day8

1. The arglist stuff was just me curing a personal gripe with functions showing up as variables in IntelliJ autocomplete. It's nice to have but my patch was incomplete and of course it means you'd have to change the API in two places going forward which is why I said to ignore.
2. I am using figwheel but that's not the motivation - I'm also using the ClojureScript fork of component. In theory, this allows the whole app and associated state to be torn down and reset to zero between code changes which is very nice. Unfortunately further testing revealed that the functions I added are hard to use in this workflow because the teardown can happen in the middle of handlers calling other handlers and shit gets funky. I think it's still valuable to expose re-frame's inner state to the end-user, however, because state is the enemy and non-standard use cases often have to control it completely (I don't just mean with re-frame). The best way to do this, however, probably isn't the functions I added so I leave that to you.
3. Good to hear!

---

On another note - I really like re-frame and using it pretty heavily has led to some other thoughts (this is all subjective and sorry to hijack the issue, can open a separate one if you agree with any of the following):

1. In pursuit of readability and "elegance" I find it much better to define a named function and then register it as a handler/subscription later, but this creates non-trivial cognitive load in having to track functions that are or aren't registered and in the fact that I can (even if I arguably shouldn't) allow the keyword event ID to differ from the function name.

2. I habitually use the trim-v middleware on almost all handlers, and I frequently get tripped up by the fact that the middleware doesn't apply to subscriptions, 99% of which look like (defn some-sub [db [_ x]]) so maybe there's a case for extending the middleware concept to those

3. I know middleware has to be declared outside the handler body so re-frame knows what to do with the return value, but this separation of logic seems less than ideal. It makes sense in the context of e.g. webapp routes because an app typically only has one set of routes so you can be relatively sure that when you find them you'll find where they're wrapped within a few lines 99% of the time. This isn't true for a multitude of handlers possibly spread across namespaces.

4. Making relatively trivial setters and getters requires 4 forms:

   1. (defn set-thing [db _])
   2. (defn get-thing [db _])
   3. (register-handler :set-thing trim-v set-thing)
   4. (register-sub :get-thing get-thing)

   Of course these can often be parameterized but that doesn't quite solve everything.

With regard to these points I think there's perhaps a case for making a few macros that covers some of them (e.g. defhandler, defsub), or doing something akin to sente for handlers i.e. a multimethod that could look something like

(defmulti handle (fn [_ _ meta] (:id meta)))

(defmethod handle :set-thing
  [db ?data {:keys [id]}]
  (assoc db :thing (do-something ?data))

(re-frame.core/set-handler! handle)

Putting all stuff not immediately pertinent to the domain problem in a map as the last arg seems a friendly and extensible solution. This doesn't play so nice with middleware as it is now, but with regard to point 3 - what if middleware could be applied in the function body with a wrap macro that told re-frame what to do in the return value metadata? Of course this still leaves the subscriptions question open.

Thoughts? Feel free to call me out on any of this and tell me I'm doing it wrong.

[mike-thompson-day8]

@lorddoig
I just added two new API functions:
- re-frame.core/clear-sub-handlers!
- re-frame.core/clear-event-handlers!
These changes will be a part of v0.4.0 (currently in develop branch).

I agree with all the other pain points you raise. I will cycle back to here in a week or so and trigger a further discussion on these issues. I've got a couple of other things to get right first.

[mike-thompson-day8]

Will this change satisfy your logging needs?
https://github.com/Day8/re-frame/wiki/FAQ#3-can-re-frame-use-my-logging-functions

This change will be a part of v0.4.0

[lorddoig]

Yup that makes sense. Just a couple of things. With respect to core/loggers being an atom - isn't logging one of those places where dynamic scoping is actually pretty defensible e.g.

(binding [re-frame.core/*log-fn* (fn [& _] (comment "Noop"))]
  (dispatch [:something-expected-to-be-noisy]))

? I'm not sure how dynamic vars work in the face of the kind of single-threaded asynchronicity we have here though, so maybe not.

It's not so pertinent but I still worry about console.group sucking up more than it's supposed to. But unless you pass grouped output to the log fn as a complete value it's hard to get around. There are some interesting approaches to this stuff in goog.debug (e.g. filterable, namespaced loggers, namespaced output grouping, etc), but it takes a little work to make it totally CLJS friendly.

[mike-thompson-day8]

@lorddoig You should now be able to any of this:

• dynamic binding
• use of good.debug

After all, you can now swap in whatever logging functions you want. These functions you provide might allow a "mute" variable to be dynamically bound to true, or they might allow you to use good.debug, that's up to you.

[mike-thompson-day8]

I have transferred your 2nd-ary notes above to a separate ticket #56 for further discussion.

But, in the meantime, do you feel the original 3 issues raised here are now handled, and can this issue be closed?