Closed urugator closed 6 years ago
Fiddle presenting the problems: https://jsfiddle.net/wv3yopo0/1212/
I've noticed that ObservableValue
extends from BaseAtom
, not from Atom
, which explains the lack of default ...Handler
callbacks.
However BaseAtom
is overall weird, it contains:
A part which states, that onBecomeUnobservable
will never get called
isPendingUnobservation = true; // for effective unobserving. BaseAtom has true, for extra optimization, so its onBecomeUnobserved never gets called, because it's not needed
At the same time provides implementation for that never called onBecomeUnobservable
(probably to satisfy the IObservable
interface)
public onBecomeUnobserved() {
// noop
}
And comment: The onBecomeObserved and onBecomeUnobserved callbacks can be used for resource management.
Can they? by whom? They are not guaranteed to get called by anything (certainly not by BaseAtom
) and onBecomeObserved
is not even defined anywhere...
The api is currently not designed to directly extend / replace / monkey patch the onBecomeObserved
, onBecomeUnobserved
handlers. Public Atoms allow attaching to this events, by passing the handlers to the constructor (new Mobx.Atom(name, observedHandler, unObservedHandler
), but other types of atoms like ObservableValue or ComputedValue have currently no official means to hook into these handlers.
I hope that explains! what is your concrete use case for which you need to attach to the handlers of an observable value?
What is your concrete use case for which you need to attach to the handlers of an observable value?
In general any resource managment - allocating/disposing connections/pools/continuous tasks/timers/data/memory/monitoring/...
Let's say you have a store which continually polls DB for some data.
When you want to start the polling? When the data are needed, right?
When are the data needed? When they're consumed by something.
When you want to stop the polling? When the data are no longer needed - when nobody is consuming the data.
So if the data are observable, how do you consume them? Well obviously by subscribing for them.
So MobX knows exactly when some data are used or not, and therefore when the resource need to be allocated/disposed.
So I was exploring if there is currently any way to get to this information (even by unofficial means).
I found out it's not possible, so I was trying to understand if there are some technical limitations (as i saw some optimizations there) or whether the Atom
API is lacking and could be rethinked/reworked.
Why componentDidMount/componentWillUnmount
is not a good solution:
Currently there is a workaround:
The idea is that you create an own Atom
with onBecome(Un)observed
handlers and synchronizes the access to this atom with the access to the observable via common accessor, like this:
get data() {
this.atom.reportObserved();
return this._data;
}
The complete example can be seen here
However this solution doesn't make much sense either, because:
Atom
to notify Mobx that this._data
is (not) being used, while the MobX already knows this information, because this._data
is already an Atom
, which reports to MobX.Atom
should represent and observable value, but the value of our atom is actually this._data
which already is an Atom
Note that you could easily map the resource allocation/disposal only to a subset of consumers of particular observable. All you have to do is to expose an adapter for these specific consumers:
// Let's say I want to run timer only when React components use the data
const data = observable("text"); // don't expose to components
const adapterForReactComponents = computed(() => data.get()) // expose to components
// hook resource management only to observable exposed to components
onBecomeObserved(adapterForReactComponents, startTimer);
onBecomeUnobserved(adapterForReactComponents, stopTimer);
@urugator the mobx utils like fromResource
or lazyObservable
don't suffice in this case for you?
Otherwise the only clean way to support this, is exposing onBecomeObserved(thing, property?, handler)
and onBecomeUnobserved
, like intercept
and observe
. That should in principle not be too hard and might be useful for debugging purposes as well.
For fromResource/lazyObservable
you need something which is already observable just by different means - something which already produces value and notifies you about the change (calls sink).
It's a basically shortcut for creating an atom, but atom creation doesn't make sense when resource allocation/disposal is just a side effect of an existing atom.
However I like to think about the onBecome(Un)Observed
simply as a MobX version of componentDidMount/componentWillUnmount
.
I have been also wondering if it would make sense or be useful to attach the handlers for a whole object graph and treat it as one. So observed handler would be called when the first observable from the graph become observed, and unobserved handler would be called when all observables from the graph become unobserved...
I have been also wondering if it would make sense or be useful to attach the handlers for a whole object graph and treat it as one
That is something you cannot reason about if you don't know about the data structure. Two objects refering to each other would kill this approach. You need a tree for that, like MST. However, if you design your data to be a tree, you just need to listen to the root node events, as accessing a deeper node should cause the root node to be accessed and observed as well in a properly design object, in other words, you can only observe d
in a.b.c.d
if you also observe a
. If a
is unobserved, you can safely assume d
is as well, unless you are leaking references to those objects
@urugator / @jamiewinder ok, there are too many good use cases where extras.onBecome(Un)Observed(thing, propName?, handler): Disposer
would be a powerful low level utility to compose new obstructions on top of it, like mentioned indeed in e.g. https://github.com/mobxjs/mobx-utils/issues/51 and https://github.com/mobxjs/mobx-utils/issues/55#issuecomment-301468627
Would you guys willing to help me out by writing unit tests for these functions?
Certainly, if I can use this to implement the keepAlive and fromAjax functions then I'll add some unit tests (though async tests can be a bit hairy). I think some basic general-case unit tests of the basic onBecome(Un)Observed should be fairly simple too.
@jamiewinder cool! Just added you to the organization, so you can create your own branches :) Ping me when ready :)
I'm about at the point where I might be able to put some time to this. Was the idea that you'd implement the low-level onBecomeObserved
and onBecomeUnobserved
utilities and we'd write the tests around the uses of them?
@jamiewinder that would be great :D. Feel free to start on tests for proper TDD ;-)
@jamiewinder @urugator Ok, took a while, but I started working on this one :-). It is less trivial then initially though. Might some input down the road, but one question so far:
I think these functions make the most sense to only be called when a real observer relationship is established (as in: some reaction is (indirectly) going to use the observable). A downside is that this will be inconsistent with the on(Un)Observed handlers of atoms, which also trigger on incidental reads.
Here are some initial tests: https://github.com/mobxjs/mobx/pull/1270/files#diff-de3e9f045a27674fe5620294772bd6c4
Sorry for the late follow up. But I agree that this is a missing low-level api that allows for very powerful abstractions.
I don't follow, what are incidental reads?
reads, but not from inside a reaction. Like computed.get()
versus
autorun(() => computed.get())
Op di 12 dec. 2017 om 02:21 schreef urugator notifications@github.com:
I don't follow, what are incidental reads?
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/992#issuecomment-350913885, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhJ3mg3mE78BvPvwjmQXjvnqFXWhMks5s_dUZgaJpZM4NZfSL .
untracked reads
Op di 12 dec. 2017 om 08:36 schreef Michel Weststrate <mweststrate@gmail.com
:
reads, but not from inside a reaction. Like
computed.get()
versusautorun(() => computed.get())
Op di 12 dec. 2017 om 02:21 schreef urugator notifications@github.com:
I don't follow, what are incidental reads?
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/992#issuecomment-350913885, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhJ3mg3mE78BvPvwjmQXjvnqFXWhMks5s_dUZgaJpZM4NZfSL .
I see, I wasn't aware that atom.reportObserved()
called from untracked context still calls the handlers...
Also documentation states that onBecomeObserved
won't be called in case reportObserved
returns false, check the last line:
if (this.atom.reportObserved()) {
return this.currentDateTime;
} else {
// apparantly getTime was called but not while a reaction is running.
// So, nobody depends on this value, hence the onBecomeObserved handler (startTicking) won't be fired
Seems weird, is this behavior even intended?
Is onBecomingUnobserved
feasible? The use case I'm imaging for this is changing the behaviour of keepAlive
such that it only kicks in when a computed is about to become unobserved for the first time rather than it being resolved up front by the keepAlive
.
@urugator yeah there is some uncannyness in the current behavior of reportObserved
/ reportUnobserved
. I can make it sensible, but that will either mean that Atom class will behave differently from built-in observables & computeds, or that specifically the hooks for atoms have slightly different semantics
Idea:
1) keep reportObserved
as is, introduce reportAccessed
:
atom.reportObserved; // explicitely stating it's observed, reaction or not, call onBecomeObserved
atom.reportUnobserved; // explicitely stating it's unobserved, reaction or not, call onBecomeUnobserved
atom.reportAccessed; // call onBecomeObserved only when inside of tracking context (actually call reportObserved)
2) if needed introduce onAccessed
handler
3) adjust internals to reflect the change
This has been fixed now in the MobX 4 branch (see #1321)
The behavior is now as follows: Introduced api:
createAtom
(instead of new Atom()
). Signature remained the same.atom.reportObserved()
will return a boolean which indicates whether the reporting happened in a reactive context. So if that is true, onBecomeObserved
will be triggered (unless it was already observed obviously). In a non-reactive context, it will return false
. It is up to the user of the atom what that means. In many cases you might want to throw an error in such case, or just don't do anything.
First some simplified self-explanatory code of what I was trying to accomplish:
The point is the ability to hook additional resource management to an existing atom, instead of creating another one and synchronizing their subscription/unsubscription through a common getter. However it doesn't work for multiple reasons, which can be expressed by following questions:
Why
onBecomeObserved
is not defined onIObservable
interface similary toonBecomeUnobserved
?Why
BaseAtom
doesn't provide default implementation foronBecomeObserved
similary toonBecomeUnobserved
? (note the comments)How so, that
getAtom(observable("hello")).onBecomeObservedHandler/onBecomeUnobservedHandler
(both) returns undefined? Shouldn't they be functions?If I replace
onBecomeObserved/onBecomeUnobserved/onBecomeObservedHandler/onBecomeUnobservedHandler
on atom obtained bygetAtom(observable("hello"))
it seems like non of it gets called whenautorun
is created/disposed (replacement is done before the reaction is created).