-
Notifications
You must be signed in to change notification settings - Fork 415
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
Adding on-dispose callbacks to a Reaction #201
Comments
As opposed to just closing over an atom? |
@erikprice I don't follow. Could you elaborate? |
Well, maybe I'm missing something. But I was thinking you could use the standard |
Just to be clear: I'd like to add a new method, That's because I have code which is given a
So this adding of a callback has to happen sometime AFTER the |
@holmsand what do think? This is pretty important because it will allow re-frame to auto de-duplicate subscriptions (think readonly cursors). If there are 100 subscriptions to the one part of the state, there should only be one Reaction observing it, not 100. I'm hesitating on doing a PR because I notice a lot of code changes in play (in this area) and I'm not sure of your current intent/timing etc. I'm guessing that you are experimenting on something (I can't quite discern overall outcome yet). |
@mike-thompson-day8 There are actually two ways to do what (I think) you want in 0.6: There's a new macro, (defn foo []
(r/with-let [value (r/atom 0)
interval (js/setInterval #(reset! value (js/Date.now)) 1000)]
[:div
@value]
(finally
(js/clearInterval interval)))) That component will show the current time stored in
The second new feature, Given a function Since "tracked functions" use |
@holmsand the new features look interesting, but neither would solve my need. (I have a general problem for which |
@mike-thompson-day8 I've had a go at adding Take a look – since I (obviously) didn't quite get your use case, let me know what you think! |
@holmsand Thanks! It is looking good so far. Will report back further in the next day or so. |
@holmsand So, this all works as advertised. And I'm happy. Many Thanks. ImplementationI note the following:
ConceptuallyFor any future users of these "dispose" related features, I note that: dispose related callbacks are not guaranteed to be called once. In certain cases, they might not be called at all and, in other cases, they may be called multiple times. Something to be aware of. Further explanation: a reaction is "disposed-of" when its number of watchers makes the 1 to 0 transition. Ie. it goes from being watched, to no longer being watched. As a result, a reaction which is never watched once, will never be disposed of (because it can't make the 1 to 0 transition, it was only ever at 0 watchers). This scenario can arise when a reaction is used conditionally within a downstream reaction like:
Assuming Also, in certain cases, a reaction can be disposed of multiple times. It is conditional use that creates this scenario. In the code above, if the value in None of this is a train smash, but it may be surprising to those using this feature. |
@mike-thompson-day8 Good point about IDisposable vs IReaction. I've done the move. I've also had another think, and changed the call to the "destroyer" function: it now gets the reaction as an argument (which may be useful, and is similar to the "normal" on-destroy). About the implicit fields: that is me trying to minimise memory use by avoiding allocation of less frequently used fields. Probably overzealous... I'll try to do some benchmarking someday to check it out. You're of course absolutely correct about on-dispose not being called exactly once. In case anyone wonders, this is by necessity. Since there are no finalizers in javascript, there's no way to know when a created reaction ceases to exist – so we cannot call the on-destroy function on a reaction that's never been used. Our only chance to do that is when the reaction no longer has any watches, so that is when on-destroy is called – even if the reaction may become active again later. |
Delivered as part of v0.6.0 |
At the time you create a
reaction
you can supply in anon-dispose
callback. All good.But I have a case in re-frame where I'd like to add an
on-dispose
callback sometime after the creation of a Reaction.Implications:
- a given Reaction might now have more than one
on-dispose
. A list?-
on-dispose
becomes a mutable field- there'd have to be an API for adding
on-dispose
callbacks.The text was updated successfully, but these errors were encountered: