re-frame icon indicating copy to clipboard operation
re-frame copied to clipboard

Feature request: Facilitate using output of subscriptions as input to handlers

Open vikeri opened this issue 7 years ago • 23 comments

Pasting from slack since this is probably a better forum to discuss these things.

First I want to apologize for not knowing enough of re-frame's internals to know if this can be solved. Would gladly help out in implementing a possible solution though.

Problem

I want to use the value of a subscription as input to a handler. Say I am developing a game and I want to display an alert box with the current score. I have an fx handler for displaying alerts :alert. The current score has a subscription :current-score that is calculated from other subscriptions that in turn are calculated from other subscriptions per the new 0.8.0 way of using subscriptions as input to other subscriptions.

Current solution

Today I have two ways of solving this:

  1. Create a subscription to :current-score in the component that triggers the :alert dispatch and dereference the sub when dispatching. This is not very elegant since the component does not actually depend on :current-score but will still be updated whenever that value changes.
  2. Extract the functions from every previous subscription in the signal graph and compose them until they only have the db as input and then use that function to retrieve the value of current score. This has the disadvantage that it will be cumbersome and error prone to extract the functions from each step and if the calculations are expensive then the performance will decrease.

Suggested solution

One way of solving it would be to have a function that took a subscription vector but did not return a subscription but just a dereferenced value directly. Not exactly pure but could be quite useful if used carefully? And then this could be sugared into the reg-event-db/fx as an optional parameter.

If it's unclear I'll happily provide some examples.

vikeri avatar Oct 18 '16 06:10 vikeri

The way I see this, the problem is not that you need subscriptions in your side-effects, but that you are not seperating your views from your event handlers. Instead of a :alert side-effect, why not have a React component that displays the score, which is shown or not depending on :is-score-alert-shown?

Example:

(defn score-alert []
  (let [show (r/subscribe [:should-show])
        score (r/subscribe [:score])]
    (fn []
      [:div {:class (str "stuff"
                         (when @show "unhide"))}
       @score])))

EDIT: added example

IsaacKhor-zz avatar Oct 29 '16 12:10 IsaacKhor-zz

@IsaacKhor Your solution is what I would use if I had an alert that was a dom element. But say that I want to use the native alert box or fire an XHR, then this does not work.

vikeri avatar Oct 30 '16 03:10 vikeri

I think I understand your problem now: :magical-variable is calculated from :other-subs, and you need :magical-variable as an input to an event handler.

In that case, I think we can somehow implement this in interceptors? Instead of inject-cofx, add some kind of inject-sub-value into the interceptor chain of the event handler?

IsaacKhor-zz avatar Oct 30 '16 03:10 IsaacKhor-zz

@IsaacKhor Yes exactly! That is an option. As long as there is a way to get the value of a subscription without creating one, I'd be happy.

vikeri avatar Oct 31 '16 01:10 vikeri

Since 0.8.0, subscribe is cached and deduped #218, so you could literally do something like:

(r/reg-event-db
  :fairies
  (fn [db _]
    (assoc db :magic @(r/subscribe [:magical-value])))

and not worry about performance. However, that is very very ugly. I'm going to work on somehow putting that into an interceptor sometime later, maybe submit a PR to include it in std-interceptors. That depends on @mike-thompson-day8 agreeing with us that this is a good idea, though.

IsaacKhor-zz avatar Oct 31 '16 05:10 IsaacKhor-zz

I just got into the same issue: I too have a subscription :magical-value that is derived in some non-trivial way from the app-db, and I need it as an input in the last event of a non-trivial dispatching chain (that is, events that dispatch other events), so it would be very unconvenient to pass it in from the view.

At the moment I'm also solving this with @(subscribe [:magical-value]) in the event handler.

While I'm very relieved that it doesn't affect performance, I don't like the looks of it and it feels very much of a hackish solution (because it's cutting the re-frame data cycle, as @danielcompton said on slack).

f-f avatar Nov 07 '16 22:11 f-f

I agree, it does look like an ugly hack. I have been working on refactoring the data in my app using specter and accessing all data through that, but re-frame's new subscription graph/composition of subs is really challenging.

As of right now, I'm working on pulling out all data access to specter on a separate layer with handlers and subs accessing all data through that layer; however, exams are coming up, and it will probably have to wait until after Christmas for me to come up with a new data layer.

That being said, I think re-frame needs a new data layer. Just being handed a db is not working well anymore; if anybody wants to work on a generalised data layer for re-frame, it would be extremely helpful.

IsaacKhor-zz avatar Nov 08 '16 01:11 IsaacKhor-zz

@IsaacKhor is was looking as specter lately too, and indeed it would be help in refactoring out a "data accessing layer" common to both subs and handlers.

With "generalised data layer" you mean some "data accessing layer" built into re-frame, or some recommended way to handle this complex cases? Are you referring to Datascript? (I think the issue with Datascript+re-frame is that it gets slow with huge amounts of data, so we'd need some simpler, faster subset of it)

f-f avatar Nov 08 '16 08:11 f-f

@ff- I agree that it feels very hackish.

@IsaacKhor I think the nested subscription model actually decreases the need for a more advanced data layer. I can access elegantly abstract away one data access from the other with different subscriptions. But if you want datascript integration you might want to check this out: https://github.com/mpdairy/posh/issues/4

vikeri avatar Nov 08 '16 13:11 vikeri

Maybe something like this?

(reagent.ratom/reaction
  (let [sub (subscribe [:magical-value])]
    (dispatch [:foo @sub])))

(r/reg-event-db
  :foo
  (fn [db [_ v]]
    (assoc db :magical-value v)))

That is, using reagent.ratom/reaction to dispatch the value of the subscription any time it changes and then using that message handler to place the result somewhere for your other message handlers to access.

Its also a bit of a roundabout way since the handler that needs the data doesn't directly access it, and the @(subscribe [:magical-value]) solution, while hackish, seems less so than this. But its another potential solution to think about, I guess. You could certainly write higher-level utility functions to do this, for example. (Note: I didn't actually test the above code...)

EDIT: A helper like this, maybe...

(defn stream-query-to-db ; better name needed ;-)
  [query db-path]
  (reagent.ratom/reaction
    (dispatch [::stream-query-to-db db-path @(subscribe query)])))
(r/reg-event-db
  ::stream-query-to-db
  (fn [db [_ db-path value]]
    (assoc-in db db-path value)))

then elsewhere:

(stream-query-to-db
  [:magical-value]             ; The query 
  [:path :to :magical-value])  ; Where in the app state to put the query's result

(r/reg-event-db
  :fairies
  (fn [db _]
    (let [magical-value (get-in db [:path :to :magical-value]]
      ...

Still very much a nasty hack and it also seems a bit weird to store a materialised view of db inside db, but at least from the users pov its convenient. (Also still untested)

danielytics avatar Jan 16 '17 12:01 danielytics

Great effort but I since the subscriptions are already cached as of 0.8.0 I think this doesn't make much sense to be honest. I would be very happy to hear from someone that understands re-frame under the hood why this is or is not a feasible feature request.

vikeri avatar Jan 16 '17 20:01 vikeri

These are quick notes for myself, for when I get back to this (in a while?). Although someone might beat me to it, and do a library. Hint. Hint. So I'm not directly addressing/answering the above conversations ... just jotting stuff down quickly so I don't forget it ...

There's two quite different issues to be addressed:

  1. The need to obtain the value from a subscription in an event handler. In effect, the need to do a one-off subscription computation (or chain of computations) so that a single value is available within an event handler for further processing.

  2. The need to trigger an event dispatch whenever a given subscription value changes. So, kinda like "whenever the value from (subscription [:xxxx "blah"]) changes, I'd like to (dispatch [:XYZ]) Reactive events. We would need the ability to create these "subscriptions" and, later, "tear them down".


Point 2 should be done via a effects handler, called say, :subscription->event. The usage of which might look like this:

(reg-event-fx 
   :some 
   (fn [cofx event] 
      {:subscription->event  {:action :create 
                              :id   42
                              :subscription  [:xxx  "blah"]
                              :event   [:this :is :what :gets :dispatched] 
                              :dispatch-event-now-with-first-value  false})))

Then later, turn it off:

(reg-event-fx 
   :some-other 
   (fn [cofx event] 
      {:subscription->event  {:action :kill
                              :id   42})))

Does anyone want to create an (effects) library for this? Does it need to be in the core of re-frame?


Regarding Point 1 (at the top) .... to make a subscription value available in an event handler, it would have to be injected into the coeffects of that handler via an interceptor. We absolutely can't have an event handler reaching out and grabbing global values.

So usage would look like this:

(reg-event-fx 
    :some 
   [(inject-sub [:query-id :param])  re-frame.core/trimv]    ;; interceptors 
   (fn [coeff event]  
       .... subscription value can be obtained from cofx 
       ))

Notice inject-sub ... it is as if it takes the value @(subscribe [:query-id :param]) and puts it into coeffect. (But where? Needs to be figured out) We should probably do this the simplest way possible at first. Just quite literally create the subscription (which would automatically utilize any currently cached values), get the value, and then dispose of it. Not that efficient but, really, does it matter?

Note this Point 2 can be handled in a library as well. It doesn't have to be in the core of re-frame, unless we want that for ease of general use.

Okay, so this ended up more than some quick notes. Ended up a quick design session and a call for volunteers.

mike-thompson-day8 avatar Jan 22 '17 02:01 mike-thompson-day8

One thing to be careful with point 2, is to make sure that you get a consistent, correct view of the data. If you are creating subscriptions that are interleaved with effects being run, I'm not sure if that is safe. May not be an issue, but thought I'd drop it here.

danielcompton avatar Jan 22 '17 21:01 danielcompton

We have a need for both point 1 & 2 above, and I took a stab at inject-sub.

@mike-thompson-day8 mentionned on slack that:

To get a general solution, we'd need to get the value out of the subscription, then despose of it. But only when it is not otherwise being used

However, my understanding of not otherwise being used is still a bit sketchy and I'd appreciate if anyone could confirm or infirm the assumptions described in the implementation below.

There also seems to be an issue in the case of a sub that'd never get used outside of inject-sub, not only defeating the cache, but creating extra work?

(defn dispose-maybe
  "Dispose of `ratom-or-reaction` if it has no watches."
  [ratom-or-reaction]
  ;; Behavior notes:
  ;;
  ;; 1. calling `reagent/dispose!` takes care of "walking up" the reaction graph
  ;;    to the nodes that the reaction was `watching` and remove itself from
  ;;    that node's `watches`'.
  ;;
  ;; 2. In turn removing a watch causes that node to dispose of itself iff it
  ;;    has no other watches.
  ;;
  ;; 3. Essentially disposing of a node will dispose of all its dependencies iff
  ;;    they're not needed by another node.
  ;;
  ;; 4. Re-frame adds an on-dispose hook to the reactions returned by
  ;;    `re-frame/subscribe` that will dissoc them from the subscription cache.
  ;;
  ;; There are some potential issues with this scheme:
  ;;
  ;; Imagine a sub that is only used by `inject-sub` and not by components, then
  ;; it will never have watches. This seems like it would be a problem if that
  ;; sub now gets injected in multiple handlers. Due to the disposal behavior
  ;; descibed above, for every handler the cofx would cause:
  ;;
  ;; - subscribe to find the cache empty
  ;; - subscribe to grab the event handler and produce a value
  ;; - subscribe to wrap the value in a reaction
  ;; - the cofx to deref that reaction
  ;; - the cofx to dispose of it (it has no watches)
  ;; - the reaction to remove itself from the cache (on-dispose hook)
  ;;
  ;; We'd basically pay for a cache miss every time we inject that sub?
  ;;

  (when-not (seq (.-watches ratom-or-reaction))
    (ratom/dispose! ratom-or-reaction)))

(re-frame/reg-cofx
 :sub
 (fn [cofx [id :as query-vector]]
   (let [sub (re-frame/subscribe query-vector)
         val (deref sub)]
     (-dispose-maybe sub)
     (assoc cofx id val))))

(defn inject-sub
  [query-vector]
  (re-frame/inject-cofx :sub query-vector))


;;; Example usage:

(re-frame/reg-event-fx
 ::my-event
 [(inject-sub [::foo/bar])]
 (fn [{:keys [db] ::foo/keys [bar] :as cofx} _]
   ;; bar is now bound to a deref'd value of ::foo/bar
   ))

julienfantin avatar May 04 '17 21:05 julienfantin

Also took a stab at implementing @mike-thompson-day8 suggestion for point 2.

There are a couple of differences here:

  • The fx accepts a val->event fn to generate the event from the subscription's value
  • There is no option to prevent dispatching the first value, this is due to reagent/track! always dispatching now by default and reagent/track being lazy. I'm not sure what would be a clean way to provide the user-defined behavior using track! without having to keep some state to count updates.

Would appreciate some feedback esp on correctness. I was also wondering if the api might be cleaner by defining different fxs for registration and disposal rather than relying on the :action key?

(defonce tracks-register (atom {}))

(defn new-track
  "Create a new reagent track that will execute every time `subscription`
  updates.

  If `event` is provided will always dispatch that event.

  If event is nil, `val->event` is required and will be invoked with the latest
  value from the subscription. It should return an event to dispatch or nil for
  a no-op.

  "
  [{:keys [subscription event val->event]}]
  {:pre [(vector? subscription) (or (vector? event) (ifn? val->event))]}
  #?(:cljs
     (ratom/track!
      (fn []
        (let [val @(re-frame/subscribe subscription)]
          (when-some [event (or event (val->event val))]
            (re-frame/dispatch event)))))))

(defmulti track-fx :action)

(defmethod track-fx :register
  [{:keys [id] :as track}]
  (if-some [track (get @tracks-register id)]
    (throw (ex-info "Track already exists" {:track track}))
    (let [track (new-track track)]
      (swap! tracks-register assoc id track))))

(defmethod track-fx :dispose
  [{:keys [id] :as track}]
  (if-some [track (get @tracks-register id)]
    #?(:cljs (do (ratom/dispose! track) (swap! tracks-register dissoc id)) :clj nil)
    (throw (ex-info "Track isn't registered" {:track track}))))

(re-frame/reg-fx :track track-fx)

;;
;; Example
;;

(re-frame/reg-event-fx
 ::start-trigger-track
 (fn [cofx event]
   {:track
    {:action       :register
     :id           42
     :subscription [::sub  "blah"]
     :val->event   (fn [val] [::trigger val])}}))

(re-frame/reg-event-fx
 ::stop-trigger-track
 (fn [cofx event]
   {:track
    {:action :dispose
     :id     42}}))

(comment
  (do
    ;; Define a sub and the event we want to trigger
    (defonce foo (ratom/atom 0))
    (re-frame/reg-sub-raw ::sub (fn [_ _] (ratom/make-reaction #(deref foo))))
    (re-frame/reg-event-db ::trigger (fn [db val] (println "Trigger" val) db))
    ;; Start the track
    (re-frame/dispatch [::start-trigger-track])
    ;; Update the ::sub, will cause ::trigger to run
    (swap! foo inc)
    (swap! foo inc)
    (swap! foo inc)
    ;; Stop the track, updates to ::sub aren't tracked anymore
    (re-frame/dispatch [::stop-trigger-track])
    (swap! foo inc)))

julienfantin avatar May 05 '17 04:05 julienfantin

This way (untested) of writing the cofx is perhaps cleaner because it doesn't involve reaching into the internals of reagent (as much). But it will be slightly less efficient (a reaction is made and thrown away):

(re-frame/reg-cofx
 :sub
 (fn [cofx [id :as query-vector]]
   (let [r   (reagent/make-reaction (fn [] @(re-frame/subscribe query-vector)))
         val (deref r)]
     (reagent.core/dispose! r)    ;; the subscription will be unwound if r is the only watcher
     (assoc cofx id val))))

Repeat, this is untested. And perhaps not even a good idea.

mike-thompson-day8 avatar May 05 '17 14:05 mike-thompson-day8

@mike-thompson-day8 interesting, that does look simpler too.

I also had some comments about a potential issue if the sub that's injected is never used in a component.

If that assumption was correct, it looks like the problem would persist with this solution. Could you confirm that it's an issue and whether this could be worked around?

julienfantin avatar May 05 '17 14:05 julienfantin

@julienfantin

I also had some comments about a potential issue if the sub that's injected is never used in a component.

Your "Behavior notes" 1 to 4 are correct. So too are your notes about the expense of a "cache miss". As you speculate, there will be the overhead of creating a subscription, adding to cache etc, and then pulling that structure down again.

I don't know any easy way to avoid this without significant surgery to the way reg-sub is used to define graph nodes. Currently, reg-sub and subscribe are designed to create signal graphs which exist for some time (and not simple function call graphs). So we currently need to instantiate sufficient signal graph to obtain a value, and then remove that signal graph.

My version is slightly less efficient again because it pays the price of creating a reaction and tearing that down too (your code is more efficient but depends on implementation details of Reagent).

But ... I'm guessing for most cases the subscription will already exist and be cached, making the value readily/efficiently available.

mike-thompson-day8 avatar May 06 '17 21:05 mike-thompson-day8

Thanks for the review @mike-thompson-day8

It seems a good compromise would be to have the co-fx log a warning if it disposes of an injected sub. If the cache miss were to happen inside a handler that's fired often like on-mouse-move I'd like to know about if and consider whether it's actually hurting performance. In cases where it's harmless enough I might decide to ignore and have an option to silence warnings : (inject-cofx :sub ^{:warn-on-dispose false} [::my-sub])

Also curious if you think reagent/track! is the right approach for the subscription to event fx?

julienfantin avatar May 07 '17 22:05 julienfantin

We shipped the inject cofx in a pre-release of our re-frame-utils. People following this issue might want to check it out!

julienfantin avatar Jul 25 '17 12:07 julienfantin

@julienfantin Sweet! I'll probably check it out. Wouldn't mind some docs on how to use it though 👍

vikeri avatar Jul 25 '17 15:07 vikeri

@vikeri cool! Docs are still on the todo list, but the fx handler for inject has a decent docstring showing usage example as well as some behavior notes that you should be aware of.

Also there are tests that may provide some more context.

julienfantin avatar Jul 25 '17 16:07 julienfantin

I've recently updated the FAQ entry on this: https://github.com/Day8/re-frame/blob/master/docs/FAQs/UseASubscriptionInAnEventHandler.md

mike-thompson-day8 avatar Jul 31 '17 12:07 mike-thompson-day8

Discussion continues on #680

kimo-k avatar Aug 13 '23 03:08 kimo-k