Skip to content

Atom and onBecome(Un)Observed #992

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

Closed
urugator opened this issue May 12, 2017 · 22 comments
Closed

Atom and onBecome(Un)Observed #992

urugator opened this issue May 12, 2017 · 22 comments

Comments

@urugator
Copy link
Collaborator

urugator commented May 12, 2017

First some simplified self-explanatory code of what I was trying to accomplish:

function onBecomeObserved(observable, callback) {
  // Obtain atom from observable
  const atom = Mobx.extras.getAtom(observable);
  // Save original onBecomeObserved
  const onBecomeObserved = atom.onBecomeObserved;
  // Replace onBecomeObserved
  atom.onBecomeObserved = () => {
    onBecomeObserved(); // call original
    callback(); // call additional logic
  }
}

// Usage:
onBecomeObserved(observable, setInterval(/*...*/));
// additionally I would like to provide unobserved version
onBecomeUnobserved(observable, clearInterval(/**/));
// and property versions:
onBecomeObserved(this, "users", setInterval(/*...*/));
onBecomeUnobserved(this, "users", clearInterval(/*...*/));

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 on IObservable interface similary to onBecomeUnobserved?

Why BaseAtom doesn't provide default implementation for onBecomeObserved similary to onBecomeUnobserved? (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 by getAtom(observable("hello")) it seems like non of it gets called when autorun is created/disposed (replacement is done before the reaction is created).

@urugator
Copy link
Collaborator Author

Fiddle presenting the problems: https://jsfiddle.net/wv3yopo0/1212/

@urugator urugator changed the title Atom and onBecome(Un)Observable Atom and onBecome(Un)Observed May 12, 2017
@jamiewinder
Copy link
Member

This could really help with the suggestions I made here and here.

@urugator
Copy link
Collaborator Author

urugator commented May 17, 2017

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...

@mweststrate
Copy link
Member

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?

@urugator
Copy link
Collaborator Author

urugator commented May 23, 2017

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:

  1. It's React specific solution, which can't be used in other contexts.
  2. The presence of the component doesn't automatically mean that the allocated resource is actually being used.
  3. The resource might be used by multiple consumers (components), to manage this properly, you would have to introduce a reference counting.
  4. It's unneccessary - MobX already does this automatically and more efficiently.
  5. It doesn't fit into MobX ecosystem - it is an analogy to manual (un)subscription.

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:

  1. We're using an 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.
  2. The Atom should represent and observable value, but the value of our atom is actually this._data which already is an Atom
  3. It's clumsy because you have to make sure that this._data won't be accessed by any other means.

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); 

@mweststrate
Copy link
Member

@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.

@urugator
Copy link
Collaborator Author

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...

@mweststrate
Copy link
Member

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

@mweststrate
Copy link
Member

@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. mobxjs/mobx-utils#51 and mobxjs/mobx-utils#55 (comment)

Would you guys willing to help me out by writing unit tests for these functions?

@jamiewinder
Copy link
Member

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.

@mweststrate
Copy link
Member

@jamiewinder cool! Just added you to the organization, so you can create your own branches :) Ping me when ready :)

@jamiewinder
Copy link
Member

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?

@mweststrate
Copy link
Member

@jamiewinder that would be great :D. Feel free to start on tests for proper TDD ;-)

@mweststrate
Copy link
Member

@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.

@urugator
Copy link
Collaborator Author

I don't follow, what are incidental reads?

@mweststrate
Copy link
Member

mweststrate commented Dec 12, 2017 via email

@mweststrate
Copy link
Member

mweststrate commented Dec 12, 2017 via email

@urugator
Copy link
Collaborator Author

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?

@jamiewinder
Copy link
Member

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.

@mweststrate
Copy link
Member

mweststrate commented Dec 13, 2017

@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

@urugator
Copy link
Collaborator Author

urugator commented Dec 13, 2017

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)
  1. if needed introduce onAccessed handler
  2. adjust internals to reflect the change

@mweststrate mweststrate mentioned this issue Feb 23, 2018
38 tasks
@mweststrate
Copy link
Member

This has been fixed now in the MobX 4 branch (see #1321)

The behavior is now as follows:
Introduced api:

  1. `onBecomeObserved(target, prop?, handler): disposer
  2. `onBecomeUnobserved(target, prop?, handler): disposer
  3. Those handlers are only called if atoms get observed while being used in a reactive context
  4. Creating atoms is now done through createAtom (instead of new Atom()). Signature remained the same.
  5. On custom created atoms, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants