search

day8 / re-frame

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

Ordered Effects #639

Open mike-thompson-day8 opened 4 years ago

mike-thompson-day8 commented 4 years ago

If I had my time over again, I would make reg-event-fx return a seq of effects, rather than a map of effects. The resulting effects DSL would be more expressive, at the cost of some nesting.

I have previously teased out the issues a bit here: https://gist.github.com/mike-thompson-day8/1388e68391f4c6c1373c9555c08c7114#using-data-litterals

Advantages:

  1. It gives a way to order effects (not that useful, but an interesting property)
  2. no need for dispatch-n , just use :dispatch multiple times (this same -n issues comes up a lot with other effects like http GETs etc)

Disadvantages:

  1. slightly noisier, visually, with the additional structural nesting
  2. is a breaking change

It might be possible to work around the breaking change problem by introducing a new reg-event-fx2 which expects the alternatively structured effects return. Allowing us to leave the old reg-event-fx alone. But it isn't all sunshine and roses - we'd end up with interceptors which worked with one kind of effects, but not the other. So then there would be v1 interceptors and v2 interceptors. One worked with one kind of effect but not the other.

@olivergeorge has made an interesting suggestion which fixes the "breaking change" side of things, at the expense of a tiny bit of boilerplate. He suggests introducing a new :fx effect for use with reg-event-fx which can take seq of other effects. 

(reg-event-fx 
   :token 
   (fn [{:keys [db]} [_ id]]
      {:db    (assoc db  :something id)
       :fx    [{:dispatch [:blah]}                             ;; <-- :fx takes a seq of effects
               {:dispatch [:other]}
               {:http  {:url   "http://abc.com/endpont  :method "GET"}}]}))

Notice the new :fx effect for which a seq of other effects is provided.

Question: if done this way, what is the ordering between :db and those in :fx? :db first? Probably.

Question: I regard :db to be a normal effect (it is just a side effect on app-db), so it could be put into :fx too. (What happens if it is not, what is the ordering?)

(reg-event-fx 
   :token 
   (fn [{:keys [db]} [_ id]]
      {:fx  [{:db    (assoc db  :something id)}
             {:dispatch [:blah]}
             {:dispatch [:other]}
             {:http  {:url   "http://abc.com/endpont  :method "GET"}}]}))

Question: maybe to reduce noise, just pairs like this

(reg-event-fx 
   :token 
   (fn [{:keys [db]} [_ id]]
      {:fx  [:db      (assoc db  :something id)   ;; <-- not a map, just a pair
             :dispatch [:blah]
             :dispatch [:other]
             :http     {:url   "http://abc.com/endpont  :method "GET"}]}))

At the moment, my inclination is this arrangement (which is slightly hybrid):

(reg-event-fx 
   :token 
   (fn [{:keys [db]} [_ id]]
      {:db  (assoc db  :something id)   ;; <-- keep :db seperate
       :fx  [:dispatch [:blah]                  ;; <-- use pairs
             :dispatch [:other]
             :http     {:url   "http://abc.com/endpont  :method "GET"}]}))

Conditional Exclusion

(reg-event-fx 
   :token 
   (fn [{:keys [db]} [_ id]]
      {:db  (assoc db  :something id)  
       :fx  [{:dispatch [:blah]}
             (when cond? {:dispatch [:other]})       ;; <-- conditionally exclude this effect.  nils ignored? 
             {:http     {:url   "http://abc.com/endpont  :method "GET"}}]}))

Or if pairs are used .... 

(reg-event-fx 
   :token 
   (fn [{:keys [db]} [_ id]]
      {:db  (assoc db  :something id)  
       :fx  [:dispatch [:blah]                 
             :dispatch  (when cond? [:other])       ;; <-- conditionally exclude this effect.  nils ignored? 
             :http     {:url   "http://abc.com/endpont  :method "GET"}]}))

As you can see, If paris are used, then It isn't neat to conditionally omit both. Only the value part of the pair could be made nil. This could be a bit clumsy because certain effects actually have a value nil value. Eg; :go-fullscreen

olivergeorge commented 4 years ago

Thanks for putting this together.

I vote this :fx form

(reg-event-fx 
   :token 
   (fn [{:keys [db]} [_ id]]
      {:db    (assoc db  :something id)
       :fx    [{:dispatch [:blah]}                             ;; <-- :fx takes a seq of effects
               {:dispatch [:other]}
               {:http  {:url   "http://abc.com/endpont  :method "GET"}}]}))

My argument being

To my mind, facilitating handler composition idea is a huge unlock for re-frame. The current handler calling convention complicates reuse.

For example (below) but this is taking it to a logical extreme which might not be justified with such simple handlers.

(defn set-something [m]
  (assoc-in m [:db :something] (get-in m [:event 1])))

(defn fetch-data [m]
  (update-in m [:fx] conj {:http {:url "http://abc.com/endpont" :method "GET"}}))

(defn dispatch-other [m]
  (update-in m [:fx] conj {:dispatch [:other]}))

(reg-event-fx2 :token (comp set-something fetch-data dispatch-other))

PS. This discussion provides some insight into the nature of :db. It thinks its useful to recognise it's domain use (state) without getting distracted by the implementation details (cofx + fx).

PPS. I'd prefer :fx to expect [{:bah 1} ...] over [:bah 1 ...] for clarity/composability reasons. It's probably a matter of taste but my sense is the CLJ world prefers maps with semantic keys over vectors & conventions.

PPPS. I think :fx could be made to simplify the :gofullscreen case (fx with no args) if it's a sticking point. Similarly, dispatch style vectors instead of maps would be compatible and possibly desirable to avoid boilerplate in fx associated with unpacking args. Perhaps the spec for :fx would become...

(s/coll (s/or :noop nil?
              :fx-0 keyword?
              :fx-m map?
              :fx-v (s/cat :id keyword? :args (s/* any?))))

PPPPS. Perhaps :db should be registered as a new type of thing allowing for different state models. State being something which passes through handlers. So it's "cofx like" in providing a value to handler and "fx like" in changing app-db on output but specifically associated with one key (e.g. :db) and by convention is passed through by handlers. That seems like a nice way to allow pluggable state management.

(reg-state :db 
  (fn [] @app-db) 
  (fn [db] (reset! app-db db)))
danielytics commented 4 years ago

Regarding db being special, would the following be valid? (Using the "pairs" syntax, although olivergeorge makes a good case for alternative syntax)

(reg-event-fx 
   :token 
   (fn [{:keys [db]} [_ id]]
      {:db  (assoc db  :something id)
       :dispatch  [:foo]              ;; <-- using an "old-style" effect
       :fx  [:dispatch [:blah]
             :dispatch [:other]
             :http     {:url   "http://abc.com/endpont  :method "GET"}]}))

If yes (it is valid), then DB isn't really treated as special and it should probably be ok to have multiple db effects in the fx vector too. In this case, its important to define the order (I would say, all old-style non-:fx effects first, in arbitrary order, followed by the new-style :fx effects, in the specified order).

If no (it is not valid), then its not really backward compatible at all, in which case I'd suggest just dropping the outer map and returning the fx vector of pairs directly.

olivergeorge commented 4 years ago

Using a (ref-fx :fx ...) would be backward compatible (technically) and :db, like :dispatch, could work in both places. Problem is that if :db appears twice it would cause a conflict which clobbers one of the changes.

I guess a fix for that might be to keep a list of effects which can appear only once and exclude them from consideration in the (ref-fx :fx ...) form.

mike-thompson-day8 commented 4 years ago

I've let this idea marinate for a few of days and I'm yet to see any downsides. In fact, I'm getting more sure this is a great idea. Even the key :fx works in nicely with reg-fx etc.

I will continue to let it sit for another couple of days just to see if any gremlins or improvements come to mind, before making it happen. While it is only about 5 lines of code, it is all the docs changes which I'm not looking forward to - adding this feature will lead to a change to best practice. I'll have to take on that task incrementally.

The benefits:

  1. no breaking changes
  2. ordered effects
  3. the ability to automatically handle the *-n variant of all effects (eg: no need for :dispatch-n)
  4. a more natural way to compose event handlers from smaller functions.

I'm currently favour this form:

(reg-event-fx 
   :token 
  (fn [{:keys [db]} [_ id]]
    {:db (assoc db :something id)
     :fx [{:dispatch  [:blah]}   
          (when seventies? {:hair "side burns"})     ;; nils are ignored
          {:http    {:method :GET  :url "http:/abc.com/endpoint"}}]}))

Notes:

Point 4 - Composing Event Handler

Point 4 of the benefits list (above) is the most intriguing, and the main reason I'm still dwelling on it. If we do this, I want to maximise the value we get from this approach. Now we have this idea in our head, and the smell of it in our nostrils, are there any other tweaks which would add even more value?

At a minimum, composing an event handler from smaller functions is now a lot more natural. For example:

(reg-event-fx 
   :token 
  (fn [{:keys [db]} [_ id]]
   (-> {:db db 
        :fx  [{:dispatch [:token2]}]}                 ;; base structure
      (start-ui-throber)        ;; <-- might assoc into `:db`
      (GET "http://abc.com/endpoint")      ;; adds to `:fx`
      (POST "http://blah.com/endpoint")))  ;; adds to `:fx`

Notes:

Compare the code above with the existing approach below which tends to encourage "effects via data literals". 

(reg-event-fx 
   :token 
  (fn [{:keys [db]} [_ id]]
    {:db (assoc db :twirly true) 
     :dispatch [:blah]
     :http  (list 
                  {:method :GET :url "http://abc.com/endpoint" ...}
                  {:method :POST :url "http://blah.com/endpoint" ...})}))

There is something pleasingly explicit to me about maximising the use of data literals, but composing effects via functions is also an interesting approach, and I like the choice, and I like being able to mix the approaches as I see fit.

FSM Example

I am particularly happy because this new approach will make it easier to handle Finite State machines (FSM) within re-frame. I look forward to being able to write: 

(reg-event-fx 
   :token 
  (fn [{:keys [db]} event]
   (-> {:db db :fx []}                 ;; base structure
       (fsm fsm-definition event)))    ;; process the finite state machine

where:

Why :fx and :db

I still regard :db is just another side effect - it just involves a reset! of app-db.

But, we all intuit that :db is a different kind of side effect to those in :fx.

But why? I'd enjoy getting a crisper explanation for this.

It is something to do with :fx effects being independent of each other and able to be done "later". But the:db side effect need to accumulate in situ because the accumulated results might be used by the next function.

reg-event-fx2

@olivergeorge I'm unclear on your reg-event-fx2 proposal (duplicated below) 

(reg-event-fx2 :token (comp set-something fetch-data dispatch-other))

Are you suggesting a new registration function which sets up the {:db db :fx []} part automatically and then threads that through the supplied event handler (in your example created via comp?

p-himik commented 4 years ago

adding this feature will lead to a change to best practice

But why? If someone doesn't care about the order (I find it to be the case in 90% of the situations I encounter), why suddenly writing

{:db (assoc db :a 1)
 :fx [{:dispatch [:do-stuff-to-a]}
      {:http {:method :get, :on-success [:inc-a], :on-failure [:dec-a]}}]}

is preferred to a simpler and yet as sufficient

{:db (assoc db :a 1)
 :dispatch [:do-stuff-to-a]
 :http {:method :get, :on-success [:inc-a], :on-failure [:dec-a]}}
mike-thompson-day8 commented 4 years ago

@p-himik This really isn't about ordered effects. (Which is confusing, given the issue's title, sorry :-)). As you say, that's a minor usecase.

For one reason, it removes the need for all the *-n variants: currently there is both :dispatch and dispatch-n. The same exists for http effects. It ends up happening for most effects. (For backwards compatibility, these will hang around, but they will no longer be featured in the docs).

Also, it allows for easier composing of effectfulness-causing functions. This is the "Point 4" I talked about above. If you have one function which wants to cause a dispatch, should it be adding to :dispatch or :dispatch-n ?? In the new scheme, always just conj onto :fx

It will become "best practice" because it just gently solves a few sticking points (no one of them a show stopper). And it means it is more natural to "compose effects" (Point 4). And because it is better if there is only one recommended way to do things.

p-himik commented 4 years ago

it is better if there is only one recommended way to do things

Indeed. But I still don't really get why it has to be the :fx method. And to be honest, I don't really get why there has to be a "recommended" way. I like having a set of tools, and I like to be able to choose the tool according to the task at hand and not because it was labeled as "recommended". We have at least 3 ways to get the first element of a vector that I can think of right away, and none of them is labeled as recommended.

Yes, :fx solves two pain points which one can encounter maybe once in 10-20 instances of using ref-event-* (just to mention it again - I can only speak from my own practice. Maybe there are people for whom the number is 3 or 100). And I wouldn't whine were it the only thing that it does. Alas, :fx also introduces a trade-off - it makes things much more verbose in arguably one of the most frequent use cases, :dispatch-n:

{:dispatch-n [[:display-pop-up]
              [:start-throbber]
              [:fetch-data]
              [:start-countdown]]}
;; vs.
{:fx [{:dispatch [:display-pop-up]}
      {:dispatch [:start-throbber]}
      {:dispatch [:fetch-data]}
      {:dispatch [:start-countdown]}}

I definitely see such scenario much more often than the need to compose or order the fx. And I personally would never recommend using :fx in this case if you don't know for sure that you will need to add some other effects in the middle of those events.

Actually, writing the above gave me an idea. What if :fx accepts a seq of not only maps but also of event vectors? In that case, the above could be written as

{:fx [[:display-pop-up]
      [:start-throbber]
      [:fetch-data]
      [:start-countdown]]}

It as all the benefits of the initially proposed version of :fx but it's also even less verbose (by the whopping 8 characters, yes) than :dispatch-n. I admit, it feels a bit "dirty". But at the same time it seems practical enough to justify that. Especially given that having either a vector or a map is somewhat a common practice with effects in general, e.g. as it's the case with :http-xhrio.

mike-thompson-day8 commented 4 years ago

@p-himik

I don't really get why there has to be a "recommended" way.

The docs have to adopt a style. Best for newbies that way.

But I still don't really get why it has to be the :fx method.

I detailed the four benefits above. I also pointed out that, to me, the most interesting was "Point 4".

I'm confident that it will make it easier to implement certain aspects of FSM. For example, I can see my way clear to finishing this library: https://github.com/day8/re-frame-http-fx-alpha (which stalled because I could never work out a clean way to compose state handlers)

But, hey, it is backwards compatible, so no need to change your existing.

ikitommi commented 4 years ago

This must be evil, but would be possible:

(re-frame/reg-event-fx ::fx
  (fn [{:keys [db]} [_ fx]] (fx {:db db :fx []})))

(re-frame/dispatch [::fx (comp set-something fetch-data dispatch-other)])
kirillsalykin commented 4 years ago

What do you think about using vectors - so having the same structure as used in subs, I think it feels more consistent.

Something like this:

{:fx [[:dispatch [:display-pop-up]]
      [:dispatch [:start-throbber]]
      [:dispatch [:fetch-data]]
      [:dispatch [:start-countdown]]}
mike-thompson-day8 commented 4 years ago

@ikitommi Evil thoughts can be very instructive and helpful :-) I prefer my events to be pure data, but it is interesting and instructive that your sketch is possible.

@kirillsalykin How would you propose to do an effect like this one: https://github.com/day8/re-frame-http-fx

mike-thompson-day8 commented 4 years ago

Actions:

We'll defer introducing any new reg-event-fx2 until we have more experience using this feature.

kirillsalykin commented 4 years ago

@mike-thompson-day8

I imagine it like this:

{:fx [[:http-xhrio {:method          :post
                             :uri             "https://httpbin.org/post"
                             :params          data
                             :timeout         5000
                             :format          (ajax/json-request-format)
                             :response-format (ajax/json-response-format {:keywords? true})
                             :on-success      [::success-post-result]
                             :on-failure      [::failure-post-result]]}

Basic idea is to use vector literal instead of map literal - so it has same syntax as reg-sub Not something very important, just for consistency.

kirillsalykin commented 4 years ago

@mike-thompson-day8

If I had my time over again, I would make reg-event-fx return a seq of effects, rather than a map of effects. The resulting effects DSL would be more expressive, at the cost of some nesting.

Would it be possible to differentiate old behaviour from new based on type then? eg if reg-event-fx returns map - it is old way fx, if vector/list - then the new way?

Thanks

mike-thompson-day8 commented 4 years ago

@kirillsalykin

Would it be possible to differentiate old behaviour from new based on type then?

Yes, it would be possible to differentiate - if you knew there were two kinds of effects. But existing interceptors expect a map of effects (they don't even know to try and differentiate). They would break.

Basic idea is to use vector literal instead of map literal - so it has same syntax as reg-sub

What you suggest is possible, but I don't see any advantages. At this point, will be going with maps.

green-coder commented 4 years ago

(To answer @kirillsalykin's suggestion) I suggest to stick with maps {:dispatch [:display-pop-up]} instead of vectors [:dispatch [:display-pop-up]]:

If one day you want to pass some meta data about the effect, the map would have room for that. Maps are future-proof.

Fictional example:

{:dispatch/retry-on-fail false
 :dispatch/may-dispatch-to-service-worker true
 :dispatch [:display-pop-up]}
olivergeorge commented 4 years ago

reg-event-fx2 @olivergeorge I'm unclear on your reg-event-fx2 proposal (duplicated below)

(reg-event-fx2 :token (comp set-something fetch-data dispatch-other)) Are you suggesting a new registration function which sets up the {:db db :fx []} part automatically and then threads that through the supplied event handler (in your example created via comp?

Yes, that my point on the input side but reg-event-fx does this well enough already. (I don't think :fx order is critical but passing in {:fx []} is helpfully predictable & explicit).

Not mentioned above, but it's important to me, is that any cofx are passed in/through too.

This leads to a sticking point: the return value handling. We want :db and :fx to be handled but since cofx will be passed through (e.g. :event) we need them to be ignored.

Here's the current behaviour:

(re-frame.core/reg-event-fx ::identity identity)
(re-frame.core/dispatch [::identity])

Console error: re-frame: no handler registered for effect: :event . Ignoring.

Questions

Option 1: Document a handler wrapper workaround If documented, it's not terrible to "do less" and simply point at a way to workaround the issue.

(defn wrap-handler [f]
  (fn [& args]
    (-> (apply f args)
        (select-keys [:db :fx]))))

Option 2: Move error into :fx (e.g. don't throw on unknown top level keys) Modifying re-event-fx so that it doesn't throw "no handler registered for effect" errors if the return map includes additional keys (like :event). Additionally, do log errors for "no handler registered for effect" associated with the :fx value.

Option 3: New registration option Leave reg-event-fx alone and produce a very similar successor but with better aligned error checking sematics.

I vote for Option 2: refactor the error checking in reg-event-fx

kirillsalykin commented 4 years ago

@mike-thompson-day8 @green-coder probably you are right and a map is the way to go. Thanks for the points.

vvvvalvalval commented 4 years ago

@mike-thompson-day8 I've got several remarks, I'll make one comment for each.

This first remark only expresses agreement, and provides supporting arguments.

1. Effects are better conj'd than assoc'd

I agree with the idea of a sequence of effects in :fx. As apps become more sophisticated, a natural requirement is that effects get assembled by various parts of the program that don't know about each other (motivating use case below). As such, an :fxsequence less limiting than map properties, because the effects involved won't collide in keyspace (the participants want to add effects, not to set them). In other words, I agree with your benefit 4.

Motivating use case: URL navigation

In an SPA, as a user navigates from a 'page' to another, the effects required are:

  1. changing a :myapp/current-nav-state property in the app-db that describes which is the current page
  2. initiating data-fetching for the new page (which might involve triggering HTTP calls and setting flags like :myapp/current-page/loading? in the app-db)
  3. potentially cleaning up some obsolete state from the previous 'page'
vvvvalvalval commented 4 years ago

2. Please do go with tuples, not maps

@mike-thompson-day8 I concur that effects under :fx should be represented as tuples, not maps. I hope to get you to reconsider 🙂

Drawbacks of maps

Maps leave no room for additional annotations on effects; tuples would be more open

Use case: As re-frame grows, for example by adding control flow facilities, callers might need to add 'meta-data' to effects, in addition to the key and value that let the reg-fx handler how to enact them.

effect-key => effect-value maps leave no room for such additional annotations, except may in a very clumsy way such as meta-data on effect-value (assuming it supports meta-data at all, which can hardly be relied on).

Maybe we can't see clearly right now what we would put in it, but that's all the more reason to leave room for it. Arguably, this very Github issue is of the 'our initial representation was not open enough' kind.

(Frankly, if I were re-inventing re-frame from scratch, I would even represent effects as maps with keys :re-frame.effect/id, :re-frame.effect/value, etc. But I understand that people might have different sensibilities regarding concision, so tuples would be fine really.)

Objection A: Effects as 3-tuples? Wouldn't this add arbitrary complexity and ambiguity to their representation?

What might worry you: assuming an [effect-id effect-value ?effect-annotations] anatomy for effects (the 3rd value being optional), it may seem unclear what should go in effect-value and what should go in effect-annotations.

Actually, I think there's a natural justification for such a representation, which makes for clear guidelines:

  1. effect-id is for identifying the effect
  2. effect-value is for use by the reg-fx handler
  3. effect-annotations is for use by the overall orchestration process of re-frame (it won't be visible to the reg-fx handler).

Objection B: We can make the :fx maps open, by requiring that there's only one effect key in them

... but wouldn't that be just more confusing?

An optional tuple argument seems like a more principled representation than a map that has exactly one effect-id key and zero or more annotation keys. Having to guess which one is the effect-id key won't be a party at all. Even if re-frame can tell by looking into its effects registry, my testing code won't.

EDIT: this is the suggestion from @green-coder above. As you understand, I disagree that this is the best way to achieve openness. In our situation, 3-tuples are more future-proof than maps.

(I think the general principle is: heterogeneous maps are open to extension; homogeneous maps are not open; semi-homogeneous maps are a tarpit).

Querying :fx: Maps would make this harder than tuples

Use case: It's important to have straightforward querying for :fx: when testing our event handler, we will want to query the result and see that the expected effects are in there. I argue that maps make this more complicated that tuples.

The essence of :fx is a collection of effects. Yet a map effect-key -> effect-value is already a container more naturally suited to a collection of effects. So if :fx was a sequence of maps, it would naturally hold a sequence of sequence of effects, rather than just a sequence of effects.

Transforming :fx: same as above, but worse

Use case: in some cases we might want to manipulate effects by transforming the contents of :fx, e.g removing some black-listed effects in a particular dev context.

AFAICT, an :fx representation as a sequence of maps would make this worse in every respect, requiring much more convoluted nested transformations than a flat sequence of tuples.

Benefits of maps

... I fail to see any significant benefit?! Maybe an aesthetic benefit, by way of syntactic similarity to the original representation?

Arguably, even semantic similarity can't be invoked to advocate for maps: structurally, a sequence of tuples is more consistent with the original map representation than a sequence of maps (since a single map is a collection of pairs).

Have I missed another reason?

green-coder commented 4 years ago

I changed my mind, the argument of @vvvvalvalval is valid, triples will be more practical.

kirillsalykin commented 4 years ago

sorry for offtopic, feels like the next step is to use datascript as app-db

vvvvalvalval commented 4 years ago

sorry for offtopic, feels like the next step is to use datascript as app-db

Already doing that by embedding DataScript in app-db. It's great. But it's good to complement it with nested maps in other parts of app-db. Actually, I'm even considering partitioning the data into several DataScript dbs.

vvvvalvalval commented 4 years ago

3. About the :db effect: I recommend disallowing :db under :fx (at least initially)

@mike-thompson-day8 Here I make a case for not allowing :db effects under the new :fx sequence; perhaps surprisingly, the main argument is compatibility.

Compared to the other effects, :db is special. Arguably, it doesn't have the incremental and imperative nature that the other effects have. IMO, a :db-update effect parameterized with an app-db -> app-db function and interpreted by swap! rather and reset! would be much more similar to the other effects, but that's not what we have and probably wouldn't make us better off.

This probably explains the following problems with allowing :db under :fx:

Problem 1: conflicts

It would probably make little sense to have several :db effects under :fx, they would essentially conflict.

Problem 2: querying fx-map for the 'latest' app-db

When several :db-transforming functions are chained for assembling a re-frame effects map, each will need to know where to find an 'up-to-date' version of the app-db to transform. In today's situation, that's easy enough: (or (:db fx-map) (:db cofx)). If :db effects were allowed under :fx, that would become something more convoluted like (or (->> fx-map :fx (keep :db) last) (:db fx-map) (:db cofx)).

Problem 3: potential breakage

For the above reasons, I suspect we will want to disallow :db effects under :fx. I'm not certain about it, but I think that precisely this lacks of certainty calls for disallowing it, because that's the path of least potential breakage: start by requiring much of / providing little to users, then provide more and require less (as in: "re-frame no longer requires of you to refrain from putting :db effects under :fx, and provides an interpretation for it.").

pavel-klavik commented 4 years ago

The proposal looks good while the precise form should be decided. We are doing something similar already in OrgPad but if this gets implemented, I might reprogram to use the default. I don't really think there is that much value in setting up ordering on effects, as designed they should be independent, so they can be applied in any order. If you really need some order, you should probably combine them into a single effect doing two things sequentially.

First, most of event handlers work with a map we called effects which is the input event map. This map is transformed by all steps. In the end, I am using a special clean interceptor to remove keys such as :event to avoid error messages.

Further, I programmed most effects to be able to use them multiple times. For instance, :ws effects sends WebSocket messages. Each message is a vector where the first element is id and optional second element is a parameter. :ws effect either gets a single message, or a vector of messages (and sends all of them). To avoid overwritting existing messages, I have a function called ws/add-message which will append the message at the end.

olivergeorge commented 4 years ago

I think this is off topic but noting here as it relates to the should :fx be a list of maps or vectors which impacts fx handler calling conventions.

Most fx handlers involve a callbacks (e.g. on success dispatch x with arg y, on error dispatch z with err). It's always bugged me that there's no convention which encourages "data based" invocation. So you can visually inspect the output of an event handler and know what the callbacks are doing. I tend to pass success-v and error-v then use dispatch in my fx handlers.

Clearly this is subject which can be explored separately but establishing a convention for this amounts to constraining how fx handlers are called.

Spoiler alert: one arg (which is a map) simplifys things greatly.

Vague, half baked idea...

Tradeoffs 👍 event handlers return more data and less opaque callback fns 👍 fx handlers are less complex since they no longer do re-frame specific work 👍 fx handlers are easier to test since they don't depend on re-frame dispatch plumbing 👎 places a constraint on how fx are called

do-fx wrapper might look like this...

(defn dispatch-wrapper [v]
  (fn [& args]
    (dispatch (into v args))))

(defn dispatchify
  [{:keys [resolve reject] :as m}]
  (cond-> m
    (vector? resolve) (update :resolve dispatch-wrapper)
    (vector? reject) (update :reject dispatch-wrapper)))
mike-thompson-day8 commented 4 years ago

@vvvvalvalval

  1. Effects are better conj'd than assoc'd

Indeed, I think we are all in agreement. That's the essential point here (despite the misleading issue title which talks about ordering). And I continue to dwell on @olivergeorge ideas around the right surrounding packaging (a potential new reg-event-xxx) to even better support the process. That I find an interesting puzzle.

  1. Please do go with tuples, not maps

I find the argument that tuples are easier to process (tests, etc) compelling.

I don't find the (future) need for metadata argument that compelling - too speculative. Perhaps that's a lack of imagination on my part.

But I was somewhat on the fence anyway. I was nudged towards maps because I don't particularly like the aesthetics of nested vectors, shrug. So your first issue is enough to tip me back towards tuples. So that's now decided unless anyone can unveil a good argument for maps.

  1. I recommend disallowing :db under :fx (at least initially)

I don't feel too strongly either way. On the one hand, I'd prefer that re-frame was permissive and helpful (produces a warning but waves it through) rather than too dictatorial. My assumption is that I'm not clever enough to know all the ways this might be used despite my overly firm opinions on how it SHOULD be used :-) But I do take your point that "the hard error" approach can be relaxed later, so it is the most conservative approach while we come to understand how to use this feature.

It is a coin flip for me.

mike-thompson-day8 commented 4 years ago

@olivergeorge

consider :resolve and :reject keys when invoking an effect handler

I believe the tuple approach (in which the 2nd element is a map) does not exclude your suggestion for future consideration.

olivergeorge commented 4 years ago

If a new registration function turns out to be needed then perhaps reg-event-dbfx is a good name.

Entirely documented by the name... an event handler returning :db and :fx where db is processed first.

vvvvalvalval commented 4 years ago

I don't find the (future) need for metadata argument that compelling - too speculative. Perhaps that's a lack of imagination on my part.

@mike-thompson-day8 Here's a random idea, still on the theme of order: you could use the 3rd argument to annotate dependencies between the effects under :fx, and then have an interceptor re-order them in a way compatible to these dependencies.

I don't claim to have good support for legitimizing such an interceptor, but it gives you an idea of the area in which a 3rd argument might play.

olivergeorge commented 4 years ago

Thinking more about how we could define & differentiate top level :db / :fx handlers and things appearing under :fx key. I think I can see a way to unknot things whichout making a mess.

Perhaps we differentiate effect and fx ideas.

This seems to align well with the current code base. We need a way to register effects, to add two common "effect" handlers for :db and :fx, and a new interceptor which replaces do-fx.

(defn reg-effect
  [id handler]
  (register-handler :effect id handler))

(reg-effect :db
  (fn [db]
    (reset! app-db db)))

(reg-effect :fx
  (fn [vs]
    (doseq [[fx-key & fx-args :as v] vs :when v]
      (if-let [fx-fn (get-handler :fx fx-key)]
        (apply fx-fn fx-args)
        (console :warn "re-frame: no handler registered for fx:" fx-key ". Ignoring.")))))

(def do-effects
  (->interceptor
    :id :do-effects
    :after (fn do-effects-after
             [context]
             (trace/with-trace
               {:op-type :event/do-effects}
               (doseq [[effect-key effect-value] (:effects context)]
                 (if-let [effect-fn (get-handler :effect effect-key false)]
                   (effect-fn effect-value)
                   (console :warn "re-frame: no handler registered for effect:" effect-key ". Ignoring.")))))))

(defn reg-event-dbfx
  ([id handler]
   (reg-event-fx id nil handler))
  ([id interceptors handler]
   (events/register id [cofx/inject-db fx/do-effects std-interceptors/inject-global-interceptors interceptors (fx-handler->interceptor handler)])))

What this effectively does is demote reg-fx.

(I see a few options to facilitate backward compatibility but haven't incorporated them in this code)

mike-thompson-day8 commented 4 years ago

@olivergeorge I appreciate the idea, and I can see your point, but I'm also comfortable with just having one kind of effect handler registered via reg-fx. After that, it is a training/docs issue.

mike-thompson-day8 commented 4 years ago

We're pushing out a new release containing this change. See change log on v1.1.0

Still to be done:

mike-thompson-day8 commented 4 years ago

Many Thanks for @olivergeorge for the idea. I think this approach will mature nicely.

tangjeff0 commented 4 years ago

This is awesome! One question: should re-frame-async-flow-fx be updated as well? It follows the dispatch-n pattern.

danielytics commented 4 years ago

At first I wasn't too sure what the value of the new approach is, for composability (I mean, associng to a map is just as easy as conjing to a vector) but after converting some of my code to the new way, the value is very apparent and I love this approach. Not having to think about dispatch vs dispatch-n or otherwise checking if an effect is already present makes for some very neatly composable code! I'm really liking how clean its making my handlers. Great work :+1:

(Just wanted to add a brief experience report to let you know that this is working out really well for me in real code)

mike-thompson-day8 commented 4 years ago

@olivergeorge

Okay, time to take the next step: to extract maximum utility from this approach by creating a new reg-event-xxx which exploits the addition of :fx, to make it easier to create an event handler as a composition of smaller functions.

As I understand it, the following is effectively what you'd like to see (it currently has too much boilerplate, but let's agree on the approach before trimming down): 

(reg-event-ctx      ;; <-- I'm using the most basic wrapper to allow us to get clear on what's required
  :event-id
  (fn [{:keys [coeffects] :as context}]
    (let [initial  {:db     (:db coeffects)        ;; <-- step 1 - data structure
                    :event  (:event coeffects)
                    :fx     []}

          result   (-> initial      ;; <-- step 2 
                     function1      ;; <-- composing the event handler from smaller functions
                     function2
                     function3)

          effects  (selectkeys result [:db :fx])]    ;; <-- step 3 - extract the effects
      (assoc context :effects effects))))            ;; <--  also step 3

So you'll notice this formulation has three steps:

  1. Setup of initial (a data structure to thread through)
  2. The composition of many smaller functions
  3. A way to extract just the effects out of the data structure which is threaded through

Any new reg-event-xxx would be doing steps 1 and 3 for you, leaving your event handler to just do step 2.

Problems:

  1. This will only work when the effects are :db and :fx (which is probably okay)
  2. Should probably add a :cofx key in step 1, so fns can access other coeffects

@danielytics

I'm really liking how clean its making my handlers.

Excellent. And thanks for the feedback - good news is always welcome.

@tangjeff0

should re-frame-async-flow-fx be updated as well

Yes, it should. But still getting there. Might be a while. If you have any thoughts about backwards compatible design changes pls add them as an issue to that repo.

olivergeorge commented 4 years ago

@mike-thompson-day8 that reads fine to me.

One suggestion, perhaps initial should be coeffects to allow for handler specific interceptors to inject cofx and have them available to the composed handlers.

mike-thompson-day8 commented 4 years ago

@olivergeorge

Ahh, using your idea and adding another, I've got it. All problems solved ...

The Goal

We want to be able to "compose" an event handler from multiple smaller functions using a new registration function, called maybe reg-event-dbfx like this:

(reg-event-dbfx  :event-id   [function1 function2 function3])

The event handler is a composition of three smaller functions.

How?

The new :fx effect (the focus of this issue) makes this possible because successive functions can conj new effects into it, oblivious to what prior or subsequent functions might have done. On the other hand, functions will have to update :db in place.

Schematically (using the base wrapper reg-event-ctx) 

(reg-event-ctx      ;; <--NOTE: I'm using the most basic wrapper 
  :event-id
  (fn [{:keys [coeffects] :as context}]
    (let [initial  (assoc coeffects :fx [] :effects-keys [:db :fx] )     ;; <-- step 1

          result   (-> initial      ;; <-- step 2 
                     function1      ;; <-- composing the event handler from smaller functions
                     function2
                     function3)

          effects  (selectkeys result (:effects-keys result)]    ;; <-- step 3 - extract the effects
      (assoc context :effects effects))))            ;; <--  also step 3

So this formulation has three steps:

  1. Setup an initial data structure, suitable for threading through the functions. It brings together effects and coeffects.
  2. The composition of many smaller functions which thread initial, progressively modifying it (typically just :db and :fx keys).
  3. A way to extract just effects out of the threaded-through data

Notes:

  1. be sure to note that I used reg-event-ctx in my code above (and not reg-event-db or reg-event-fx) - this is a little known, existing way of registering an event handling.
  2. be sure to note that in step 1, I created initial by just assoc-ing into coeffects. So initial is just the context's coeffects with two keys added: :fx as an empty vector, and effects-keys which are the keys which should be later regarded as effects.
  3. During step 2, functions can conj onto :effect-keys if they need to indicate that they have added another effect key (they typically won't need to)
  4. But mostly in step 2, the functions will be updating :db and adding to :fx
  5. Be sure to notice that I extract effects-keys in step 3 (:effects-keys result)
  6. the proposition is that reframe should provide a new registration function called, say, reg-event-dbfx which will look after steps 1 and 3, allowing you, the programmer, to just provide the functions to be composed in step 2.

Outcome

You'll be able to do this:

(reg-event-dbfx       ;; <-- new 
  :event-id  
  interceptors 
  [function1 function2 function3])

Just to be clear, function1 might be something like (very simple): 

(defn start-ui-twirly
  [dbfx]
  (assoc-in dbfx [:db :twirly-flag] true))
olivergeorge commented 4 years ago

Nice.

Loving the explicit :effect-keys.

Not sure if the [fn1 fn2 fn2] form adds enough over (comp fn3 fn2 fn1) to justify itself. Best argument I can see is that it does allow for the order of execution to be "left to right" which might be more intuitive. Another argument might be improved error reporting (e.g. "function N threw an exception").

mike-thompson-day8 commented 4 years ago

We are about to implement and release reg-event-dbfx. Any objections? Speak now or forever hold your peace.

Ramblurr commented 4 years ago

My only comment is one of naming reg-event-dbfx seems confusing given existing re-frame functions. reg-event-db, reg-event-fx, and others. As a beginner/intermediate the names are so similar to seem overloaded. Just my 2 cents.

mike-thompson-day8 commented 4 years ago

For the record, my earlier schematic using reg-event-ctx can be written using the less exotic reg-event-fx 

(reg-event-fx
  :event-id
  (fn [coeffects _]
    (let [initial  (assoc coeffects :fx [])    ;; <-- step 1

          result   (-> initial      ;; <-- step 2 
                     function1      ;; <-- composing the event handler from smaller functions
                     function2
                     function3)]

      (selectkeys result [:db :fx]))))    ;; <-- step 3 - extract and return the effects

(which can be written much more tersely)

This realisation is encouraging me to wonder if we really need reg-event-dbfx. Do we just need to teach the technique, not introduce a new registration mechanism?

olivergeorge commented 4 years ago

I can see the argument in waiting until there's evidence that the boilerplate is annoying or troublesome for third party libs which want to rely on composable behaviour.

Some kind of comp-dbfx wrapper can acheve the same result for use with reg-event-fx.

niquola commented 4 years ago

Maps are better for long term design - less mental load; easy to compose and manipulate. With ordered fx you force order, even if it is not needed. Forcing order is one of sources of accidental complexity. Order in functional langs are modeled with nesting - instead of switching to vector we can make all events and fxs to be maps and add special keys for chaining. I can imagine collection of high order effects to compose fxs and events. Ideally multiple dispatches can be run in pure world as reduce with merge of fxs. @mike-thompson-day8 let's have a good reasonable complicated use case to exercise design ;)

niquola commented 4 years ago

About dispatch(-n) - are you sure it is effect at all? What is the reason for event handler to dispatch other events?

danielytics commented 4 years ago

@niquola I, too, at first thought that maps are simpler and compose just as well, but I’ve since switched to this new way and my code really is simpler. The reason is that any function I call simply takes an fx map ({:db ... :fx [...]}) and returns an fx map, if they need to update the app state they run (update-in fxmap [:db ...] ...) and if they need to add an effect they run (update fxmap :fx conj [new effect here]).

The db update doesn’t change at all, but the effects now become much easier, because if I want to add an effect to be run, I simply add it to the vector. Order usually doesn’t matter so I can just conj it without a second thought (but if order did matter, it’s now an option too). This on first look doesn’t seem any better than just associng a new effect into the old map, but consider this:

Before, each composed function needed to be aware of the existing effects in the map, or they might clobber them. Eg if you wanted to dispatch you always had to use dispatch-n in case there was already a dispatch effect (or check if it’s there). What if you want to add a http request? You had to check if there already was one in the map to not clobber it. Hopefully there was a http-n effect so you could add a second one. On that note, any time you wrote an effect, you had to provide an -n version, or make all your effects -n aware. This change makes that unnecessary and makes the effects vector explicit about what is being run.

Personally, my code got simpler this way because I don’t need to think about what effects have already been added, I just conj to the end of the list. I have a function that sets up a http request and handles a response event. This sets up a number of dispatches and effects and the logic for this got much nicer with this change. If the dbfx sugar functions get added, I will likely be changing all of my handlers to using those.

I would suggest trying it out for any complex composed handlers you may have and see for yourself. If you don’t have any, it’s backwards compatible, so you can just stick with the old way. No need to change anything in that case.