Skip to content
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

mobx.observe does not call callback when computed property changes #486

Closed
fenduru opened this issue Aug 15, 2016 · 10 comments
Closed

mobx.observe does not call callback when computed property changes #486

fenduru opened this issue Aug 15, 2016 · 10 comments

Comments

@fenduru
Copy link

fenduru commented Aug 15, 2016

I am trying to create an object that has a number of computed properties on it, and I would like to observe whenever any of the computed properties change using mobx.observe(obj, callback). This works fine if I explicitly pass the propertyName argument, however not if I try to observe the parent object itself.

I do see the docs mention that observe(obj.name, callback) is different than observe(obj, 'name', callback), but I believe this is different because I do not want to provide the name at all, and instead I want to listen for changes to any property on the parent object.

I am expecting mobx.observe(obj, callback) to call callback more than or equal to the number of times mobx.observe(obj, 'property', callback) would call callback.

Code

var bar = mobx.observable(10);

var obj = mobx.observable({
    foo: 1,
    doubleFoo: function() { 
        return this.foo * 2;
    },
    doubleBar: function() {
      return bar.get() * 2;
    }
});

mobx.observe(obj, function() {
  console.log('no property:', obj);
});

mobx.observe(obj, 'doubleFoo', function() {
  console.log('doubleFoo:', obj.doubleFoo);
});

mobx.observe(obj, 'doubleBar', function() {
  console.log('doubleBar:', obj.doubleBar);
});

console.log('set foo');
obj.foo = 2;

console.log('set bar');
bar.set(20);

Console output

set foo
doubleFoo: 4
no property: Object {$mobx: ObservableObjectAdministration}
set bar
doubleBar: 40

Expected output

set foo
doubleFoo: 4
no property: Object {$mobx: ObservableObjectAdministration}
set bar
doubleBar: 40
no property: Object {$mobx: ObservableObjectAdministration}

jsFiddle

@hellectronic
Copy link
Contributor

I never used observe(), but I would not expect the second "no property: " output.
You are not changing obj if you call bar.set()

@fenduru
Copy link
Author

fenduru commented Aug 15, 2016

I understand your point. I guess I just find it odd that observe(obj, callback) observes all non-computed properties rather than all of the properties.

@hellectronic
Copy link
Contributor

observable() makes every property in the obj oberservable

If value is an object without prototype, all its current properties will be made observable. See Observable Object.

That is why setting obj.foo = 2; fires

@urugator
Copy link
Collaborator

urugator commented Sep 1, 2016

I have to agree with @fenduru, current behavior doesn't make sense. The mobx.observe(obj) should emit change event on computed props as well.
However the expected output should actually contain three
no property: Object {$mobx: ObservableObjectAdministration}" msgs.
First for foo, second for doubleFoo and third for doubleBar.

I wonder why it works how it works, I hope it's not because of some technical limitation...

EDIT: You might want to change the title, the dependency on external observable is not the cause of the problem.

@fenduru fenduru changed the title mobx.observe behaves differently if computed property depends on external observable mobx.observe does not call callback when computed property changes Sep 1, 2016
@mweststrate
Copy link
Member

Hmm good point, I usually don't consider computeds as 'full members', they are not enumerable as well for example. But at least if the event is emitted the user is able to choose himself to ignore the change. I'll mark this for 3.0 candidate, as the change might be breaking for people.

@mweststrate mweststrate mentioned this issue Dec 27, 2016
19 tasks
@mweststrate
Copy link
Member

mweststrate commented Dec 31, 2016

Tinkered about this a bit more, (already started to change it) but I think the current behavior is correct

  1. (not so relevant) with the current approach only events are fired for enumerable members, which is also kinda consistent
  2. (relevant) reactions are about reaction to new values, whenver they become visible (after transaction etc), while observe is about reaction to new mutations when they are applied (for primitive this is just a new scalar, but for arrays it produces slices etc). A computed value is not the result of a single mutation, but of a new value being produced. This distinction is quite relevant in distributed systems where only mutations are propagated, and not the (output of computed) values
  3. One could argue that for that reason it should not even be possible to observe a computed value (in the implementation it is even an autorun! But since that one is available, as mentioned earlier it is always possible to observe computed values explicitly (e.g. observe(object, 'computedPropName', listener))

@urugator
Copy link
Collaborator

urugator commented Dec 31, 2016

A computed value is not the result of a single mutation, but of a new value being produced

Isn't that newly produced value stored/cached to "box" (like boxed.set(producedValue)), which is basically the mutation ... so the value isn't the result of single mutation, but "caching" the value to a property (box) is that single mutation, which we are observing for?
I really don't have a clue, just speculating, thinking loud...

Maybe the observe works fine, but we are just missing the functionallity of observe (new mutations), which would have the attributes of reaction (new values).
You know with observe you can:

  • subscribe for objects (not possible with reaction, you have to subscribe for specific props via dot notation)
  • get the details about the change (oldValue/event) (reaction provides only the new value)

Btw, isn't MobxReact.observer with it's functionallity more close to a reaction then observe (so the naming is kinda misleading)?

@mweststrate
Copy link
Member

@urugator yep good thoughts. I'm not entirely sure about this one either. I think for now the status quo is fine until the community has a stronger opinion about this :) So far there are good use cases for both solutions.

And btw, yeah observer totally doesn't relate to observe indeed, it is like a reaction (autorun actually)

@urugator
Copy link
Collaborator

urugator commented Jan 2, 2017

@mweststrate However, there is one thing I would definitely change and that is unifying the listener arguments. Its sometimes an event and sometimes prev/new value, I think it's confusing and impractical and even the documentation has trouble to make that clear:

listener: callback that will be invoked for each change that is made to the observable. Receives a single change object describing the mutation, except for boxed observables, which will invoke the listener two parameters: newValue, oldValue.

(exception isn't just boxed observables, but also computed and props) and then:

The callbacks of intercept and observe will receive an event object which has at least the following properties:

object: the observable triggering the event
type: (string) the type of the current event

These are the additional fields that are available per type:

@mweststrate
Copy link
Member

@urugator good point! It was actually on my todo list somewhere but I forgot to open an issue for it. Added a task for that in #679

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

4 participants