Don't autoconvert function without arguments into getters

[andykog]

For me, one of the least pleasant parts of mobx is automatic converting function without arguments to getters.

The problem

// ordinar mobx store
class KittensStore {
  @observable myKittens = [];
  addKitten(kitten) {
    this.myKittens.push(kitten);
  }
}

// somewhere in ui:
kittensStore.addKitten({
  name: "Kitty",
  sayMeow: asReference(() => alert('meow')), // ui have to know about store internals
                                             // and that the values will be mobxified
  sayText: (text) => alert(text), // doesn't need asReference, feels inconcistent
  onClick: () => alert('hello'), // damn, I've forgotten about autoconverting
});

Proposition

It would be really nice not to make such a bold assumption. We can introduce special modifier computed, so if one really want to create getter, he can show it explicitly:

extendObservable(this, {
  count: 0,
  doubleCount: computed(() => this.count * 2),
});

Pros:

• No magic
• More consistent API
• Any data can be accessed the same way after mobxification
• Less surprises for the new developers
• Easy to find and rewrite 'asGetter' accordingly (for example, during upgrading to es2015)

Cons:

• Breaking change

PS: I know about asFlat

UPD
changed asGetter -> computed

[urugator]

You can't expect that any arbitrary object coming from UI can be automatically turned into observable.
It's not just function references ... also arrays will be turned into objects, so all Array.isArray(kitten.toys) in UI would be failing without MobX knowledge.
As a side effect you're probably observing properties, you don't even want to observe, like sayMeow.
Also with your proposal, the problem in your example can just turn into this:

let kitten = { _name: "" };
kitten.name = asGetter(() => kitten._name.trim()); // ui still need to know about mobx internals
kittensStore.addKitten(kitten);

Btw, I think that asGetter() should actually be computed()

However, I see your point about consistency, I just don't think it's a good example.
I feel as well that all functional properties should be handled the same way, no matter how many arguments they have, so let me offer another proposal:

Forbid using functional properties with length > 0 inside objects passed to observable/extendObservable

So, if there is a function in observable/extendObservable call, it's either a getter or it must be wrapped into asReference(), making everything clear and simple.

[andykog]

Array.isArray(kitten.toys) in UI would be failing without MobX knowledge

Yes, but thats not really desired behaviour, it happen only because we have no way to make it return true in current JavaScript.

As a side effect you're probably observing properties, you don't even want to observe, like sayMeow

Yes, I'm observing reference to sayMeow. I want to observe everything inside observable object, why not?

Also with your proposal, the problem in your example can just turn into this [code] ui still need to know about mobx internals

There is no any necessity to define mobx computations:

let kitten = {  _name: "" };
kitten.getTrimmedName = () => kitten._name.trim();
// or:
Object.defineProperty(kitten, 'name', { get: () => kitten._name.trim() });

If I really want to do this optimization, I can do it explicitly in the store:

class KittensStore {
  @observable myKittens = [];
  addKitten({ getTrimmedName }) {
    this.myKittens.push({ name: computed(getTrimmedName) });
  }
}

Btw, I think that asGetter() should actually be computed()

Agree

---

Summary

My point is that if user turns object of shape { key: () => string } into observable, its shape have to stay the same instead of becoming { key: string } , or there must be very strong reasons for breaking this expectation.

[urugator]

My point is that if user turns object of shape { key: () => string } into observable, its shape have to stay the same instead of becoming { key: string } , or there must be very strong reasons for breaking
this expectation.

Let me rephrase it:
When user EXPLICITELY specifies properties, which should be observable by calling observable()/extendObservable(), than he expects that properties containing a function should be observed as reference?
Why would he expect it?

• he's explicitely saying the property should turn into observable, so logically the property have to be transformed in some way to do the the observable magic
• the properties containing plain objects are NOT observed as reference
• the properties containing arrays are NOT observed as reference
• only classes and primitives are observed as reference

When I started with MobX, I did something like this:

const control = observable({
  value: "intialValue",
  validate: () => console.log("validating...")
});

What was my expectation? Not that the property validate will be observed as reference, but that the property won't be observed at all, because it contains a function....silly me :)

My point is:
It's not about what user expects, it's about sane defaults.
Most of the time, the state is not represented by a function itself. However it's quite common to have a derived (computed) state which is represented by a function. Therefore it might be handy to automatically turn functions into computed values by default (that's the strong reason you're looking for, I quess)

To me personally, it doesn't really matter if I have to write computed() or asReference(). Neither of those defaults will work for any plain object, but I think that I would have to write computed() far often then asReference().

Still, I don't like making a difference based on the number of arguments, there is enought magic involving property type (I don't like this magic either, but I kind of see and appreciate it's practicality).

[andykog]

When user EXPLICITELY specifies properties, which should be observable by calling observable()/extendObservable(), than he expects that properties containing a function should be observed as reference? Why would he expect it?

I think it's quite natural to expect object to stay the same shape after adding to ObservableArray by default. If the key is a function, it can be observed by reference, as well as RegExp or Date.

It's not about what user expects, it's about sane defaults.

Sane defaults should correspond to what user expects if we're making user-friendly library.

I think that I would have to write computed() far often then asReference()

Explicit code is often more verbose. I'm using es2015 classes for representing app state, so I have to use @computed anyway, and it doesn't bring me any amount of discomfort. When you use computed() you are doing things. When you use asReference() you prevent mobx from doing undesired things. Thats the difference.

[andykog]

It would be very interesting to hear @mweststrate thoughts on this.

[urugator]

Sane defaults should correspond to what user expects if we're making user-friendly library.

I agree only partially. Everybody can expect something else. An expectation can turn into something absolutely impractical in the end.

I think that the difference between our views is, that you see observable()/extendObservable() as something which can turn any domain object, with all it's complexity, into basically the same object, but somehow sanely observable.
While I see it just as initializer or descriptor for observable fields.

If explicit code doesn't bring you any discomfort, why don't you turn your kitten and it's properties into observable explicitely, like you have to do with your classes? Both are quite smart objects, they are not just dumb state holders, so is it because it's not possible to do this observable(KittenStore) (applying the same rules as for plain object)?

you prevent mobx from doing undesired things

It's undesired for your plain objects, which you want to turn into observables.
For me it's desired most of the time, because I don't turn smart objects into observables just by calling observable(smartObject). I don't believe it's possible to do that reliably, no matter what defaults will be used for conversion into observable.
I haven't had a need for asReference() yet, because as I said it's very rare to have a state represented by a function. However I use computed properties very often, so it's a convenient to me, that I don't have to write computed() inside obsevable/extendObservable, when computed() is most of the time what I actually want.

But as I stated earlier, It really doesn't matter to me, what will be the default behavior, as far as it will be consistent. With that in mind, if there are scenarios, where it would be beneficial, than it's worth considering.

[andykog]

Renaming observable #470 is good opportunity to change it's behaviour

[mweststrate]

This magic behavior of computed functions bothers me for a very long time already. But personally didn't find a more satisfying approach. Just some thoughts

1. I plan to forbid multi arg functions at some point completely, as now you should pass multi args as action(bla) to extendObservable. computed() would be very consistent with that.
2. Thought for a while on introducing a separate extendComputed, but I don't really like that either
3. What makes the function to computed conversion more ugly is the fact it changes the type from prop: () => T to prop: T during the conversion. I don't like that. (That is why @computed is always accompanied with get, if that would be omitted, the type system would break as a result of this same conversion). Changing this extendObservable behavior would roughly create the type extendObservable(a: A, b: B): A & B, except that modifiers are still not expressible in a type system.
4. converting functions to computed is a very convenient from a convention-over-configuration perspective. It is the most use scenario hence a sane default. But also surprising for the not mobx-aware person.
5. Accidentally marking functions as observable without using them as such, as @urugator did initially, is in principle harmless although a little waste of memory.
6. computed would nicely fix Introduce support for setters for computed properties #463 for non ES5 only approaches.
7. See Rename usage of "observable" to not confuse with TC39 observable #470, if observable -> state, it would be weird to have computeds in there at all. If observable -> tracked, then it would make a lot of sense :)
8. Probably unrelated, I dislike that you cannot use arrow functions in observable (or extendObservable if extending anything but this), as it would bind to the wrong context.
9. For modifier consistency, asComputed? I like computed better though. Maybe other modifiers should be renamed as well, like: shallow instead of asFlat etc
10. @mattruby as our true ES5 spokesman, what's your 2 cents ;-)

So bunch of thoughts, no conclusion yet :) Could slowly move towards encouraging computed, until not using it is completely deprecated at some point

[mattruby]

@mweststrate I do like the automatic computed in observable and extendObservable. I use that convention often. In my experience, it has taken others a little while to understand what's going on though. If things are changing, I'd probably go the same route as action and asMap: mobx.computed() would make the most sense to me. I'd also be cool withasMap to map.

[urugator]

Accidentally marking functions as observable without using them as such, as @urugator did initially, is in principle harmless although a little waste of memory.

It's not possible to use them as such, because they become a getter (computed prop).

Could slowly move towards encouraging computed, until not using it is completely deprecated at some point

In that case, I think there is no point in prohibiting multi args functions. When computed would be required, than asReference would make sense to be the default for any function.

If @computed must be accompanied with get, why don't we auto convert only getters (instead of argumentless functions) into computed props?

[mweststrate]

@urugator sorry you're right. On the last question, what do you mean by getters? Anything that looks like a prop descriptor, or actual property descriptors? That might be convenient for ES6 user indeed

@andykog Thought about it, I think the current behavior should indeed be deprecated, as in unexpectly 'breaks' the type of an assignment, especially in less obvious cases like:

const x = observable({ y: null })

x.y = { z: function() { console.log("hi") }

x.y.z() // nope

[urugator]

@mweststrate Actual prop descriptor, obtained by Object.getOwnPropertyDescriptor()

extendObservable(this, {
   side: 1,
   get volume() { return this.side * this.side; }, // turns into computed prop
   viewClass: function() {....} // plain functions are simply observed by reference, arg count doesn't matter
})

[andykog]

@urugator, detecting and turning getters into computed value is awesome idea, by the way! And it resolves another issue of turning getter into static observable value.

[mweststrate]

I think the get variation might be introducing the same mistake as we currently have with the automatic conversion of functions, what if somebody actually wants to create a normal getter, not a computed?

For that reason I think I'll prefer the approach as suggested in #42, although it is a bit more verbose:

extendObservable(this, {
  side: 1,
  volume: computed(
    () => this.side * this.side,
    (v) => Math.sqrt(this.side) // optional setter
  )
})

[urugator]

@mweststrate

what if somebody actually wants to create a normal getter, not a computed

If I want a normal getter I shouldn't define it as an observable in the first place.
I mean, if I define a field in an object passed to observable/extendObservable, it must be observable somehow, so how the getter is going to be observable? as a reference to a function?

[andykog]

@urugator, normal getter will still be tracked, creating it with computed(() => x) is just matter of performance. There is no need to observe it, as it can be redefined only with Object.defineProperty but the same possibility exists for any other properties (e.g. primitive values)

I think the get variation might be introducing the same mistake as we currently have with the automatic conversion of functions, what if somebody actually wants to create a normal getter, not a computed?

@mweststrate, Can you imagine some use case when this can lead to a problem? Observable getters behave the same way as normal and performance overhead for simple getters is miserable, right? I'm not opposing, just asking.

[mweststrate]

No cannot think of such a use case, and I think it is quite easy to implement. But somehow it feels a bit arbitrary that get means computed. (as arbitrary as function() {} means computed ;-)) Or is that just me?

[urugator]

As @andykog made clear, there is no point in observing a getter field, because it's "value" can't be changed in an observable way. So a getter defined in an observable is practically unobservable (or am I missing something?), which seems a bit wierd to me.

Also, I think, it kinda makes sense to turn getter into computed, because getter is in fact a normal field, which value is a just a derivation of other field/s.
If those other fields form an observable state of the object, shouldn't their derivations form an observable state as well?
(And note, that we are explicitely marking the getter as observable, by placing it in observable object definition, so one can still create a normal getter)

Similary to:
"What if somebody actually wants to create a normal getter"?
I could ask:
What if somebody actually wants to create a normal function?
But that's not the problem we are dealing with. We don't want the function to be normal, but observable. The actual question is, observable how? As a reference or something else (computed)?
So I think, we should analogically ask in which way the getter should be observable.

And the least importantly, it's more appealing :)

extendObservable(this, {
  side: 1, 
  get volume() { return this.side * this.side }
  set volume(v) { this.side = Math.sqrt(v) }
})

EDIT:

somehow it feels a bit arbitrary that get means computed

In that case I think the following should feel arbitrary as well:
{} means observable({})
[] means observable([])

EDIT: fixed getter syntax

[mweststrate]

Note that the above syntax won't work afaik, should be get volume() { ... } which is slightly uglier.

But yes, I admit it looks tempting. Was quite easy to setup, see above commit :)

[mweststrate]

Btw I actually never realized this is valid ES5 syntax :-/

[urugator]

You have a typo in test description "... plan object ...".
Btw is setter wrapped into an action or left as is?

[mweststrate]

it is wrapped in an action indeed. thanks for the idea btw, despite my
initial scepticism I really like it! wish i had realized half a year ago
this is valid es5 ☺ I think I'm gonna push this as the idiomatic way to
create computeds without decorator

Op wo 31 aug. 2016 22:52 schreef urugator notifications@github.com:

You have a typo in test description "... plan object ...".
Btw is setter wrapped into an action or left as is?

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#421 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABvGhOHAtYwIrwU4rf5cDVMbqeRZURodks5qlemWgaJpZM4JP03A
.

[mweststrate]

Released as 5.2.1, opened a separate feature to kill the current behavior.