Can we improve the API in such a way that people will better understand what MobX does?

[mweststrate]

Can we improve the API in such a way that people will better understand what MobX does?

The discussion was initiated in the 4.0 roadmap discussion here: #1076 (comment)

For an extensive background (discussing the current API design): #649

The current api to create observables is generally well appreciated. However, currently I have one beef with it:
It doesn't help people, especially not too experienced programmers, on what MobX does.
Especially the conceptual difference between a value and a property is particulary confusing.
I think this is partially caused by the fact that @observable anything generally just works, until it doesnt :)

Some typical examples:

1. @observable buf = new Buffer();. isObservable(this.buf) returns false. Which is correct. Should have been: isObservable(this, "buf") was intended.
2. observable(thing) will make thing observable, unless it can't, in which case it will create box around the thing rather then enhancing thing.
3. People arbitrarily use this.items = this.items.filter(...) or this.items.replace(this.items.filter(...)). With @observable items = [] both will work, but it would be nice if people grabbed the difference.

[mweststrate]

For reference, current API:

@observable
@observable.ref
@observable.deep
@observable.shallow

observable.box 
observable.shallowBox
observable.object
observable.shallowObject
observable.array
observable.shallowArray
observable.map
observable.shallowMap

extendObservable
extendShallowObservable

@computed
@computed.struct
@computed.equals
@action
@action.bound

[mweststrate]

cc @urugator @jamiewinder @spion @phiresky

[mweststrate]

Maybe this makes everything a bit more explicit? Here a very first iteration of a draft proposal

Proposal

type opts = {
	depth: "deep" | "shallow" | ref" // "ref" is the default
	name: string
	comparer
}

defineObservableProp(target, key, value, opts?)

@observable(opts?)
@computed(opts?) 
@action(opts?) // name and bound

decorate(target, {
	prop: observable(opts?)
	prop2: computed(opts?)
	prop3: action
})

observable.box(value, opts) 
observable.array(value, opts) // throws on opts.depth = shallow
observable.map(value, opts)   // throws on opts.depth = shallow
observable.object(value, opts) // different from current impl, will support adding new keys

// convenience decorators:
@computed
@action
@action.bound
@observable.ref
@observable.shallow
@observable.deep

---

TL;DR

1. In essence @observable / observable(thing) will be dropped, you have to be more explicit then that
2. observable.array etc by default will use "ref" modifier instead of "deep"
3. modifiers are passed in as option, rather than having their own method (like shallowArray)
4. There is an non-decorator friendly way of decorating a type (tnx @urugator!)

Things dropped

@observable
observable(thing)
observable.shallowBox
observable.shallowObject
observable.shallowArray
observable.shallowMap

extendObservable // not sure about this one

@computed.struct
@computed.equals

[phiresky]

Sounds pretty good.

You didn't specify what the functions / annotations do exactly:

Does @observable.deep and shallow (and @observable({depth: "deep"})) throw if passed a class instance or a primitive / string? I think they should.

And why does observable.array/map throw on .depth = shallow?

Also, how would inner modifiers work here? E.g. you have an @observable.deep property but somewhere in the sub objects you want to have a shallow / ref property?

[spion]

Wrote it in the other thread, but just for reference I'll add it here too

I suggest that any proposal should involve rewriting https://mobx.js.org/getting-started.html and seeing if it still looks like a good introduction.

[urugator]

And why does observable.array/map throw on .depth = shallow?

Yes this is quite confusing ... basically there are 2 ways of how to look at the "depth" option in case of observable containers:

1. Depth is applied to the container's elements.
   In that case observable.array(val, { depth: "shallow" }) would have to return observable array with elements shallowly observable, which is not supported (and usually not needed).
2. Depth is applied to the container with it's elements as a whole.
   In that case the observable.array(val, { depth: "ref" }) would have to return boxed array, which wouldn't make sense...

"depth" option doesn't work well with type specific factories and causes confusion because of this "depth" shifting. Current "optionless" API doesn't have this issue.

Also, how would inner modifiers work here?

Modifiers only say what to do with non-observables, so if you put something "shallow" into something "deep" it stays shallow, because it's already observable.

[urugator]

I think this is mainly a documentation issue.

All tutorials and examples show this:

@observable prop = val;
//or 
observable(o);

Why current doc doesn't emphasize the same things as the proposal (references/specific types/etc)?

We will just replace @observable by @observable.deep everywhere, while not really explaining anything.

[spion]

For (1) I have a solution: separate functions for isObservable instead of overloads i.e. isObservableProperty & isObservableCollection (maybe also isObservableRecord)

For (3) I see that as a feature, not a bug. Its "transparent reactive programming" after all.

Aren't proxies going to help make it even more transparent, btw? 😀

[mweststrate]

Good points being made here :). Pretty hard to beat the current api, but there was given a lot of thoughts to that so that makes sense.

Maybe the changes should be smaller. Alternative idea. Agreeing here with @urugator that depth will be always confusing.

1. throw error on observable(primitiveish) -> use observable.box
2. separate isObservableProperty as @spion suggested
3. Either change the default deepness of the other methods. So observable.array and observable.deepArray instead of the current observable.shallowArray and observable.array. Or, have a deep = false parameter as second argument (or options object). observable(thing) could remain deep by default, or would that be confusing?
4. @observable could stay, but in strict mode throw that .ref or one of the others should be used?
5. Rename observable.box to observable.ref? Would that be more consistent @urugator ?

[jamiewinder]

I'm look through this again later, but a few thoughts:

• Shallow observability by default is good. I don't think I actually use deep observability in my code anywhere!
• I like observable.box over observable.ref as it implies 'the contents of the box is observed' and it's the box rather than the contents that you're observing. observable.ref(1) implies 'I refer to 1' which I don't think makes particularly makes sense or stresses what is happening.

I love the current API, but given how I tend to use MobX 'shallow by default' makes a lot of sense.

[urugator]

1. throw error on observable(primitiveish) -> use observable.box

Imho observable(any) should not exist or it should accept anything.

2. separate isObservableProperty as @spion suggested

Agree

3. observable.array and observable.deepArray

Agree (in case ref/shallow is default)

4. @observable could stay, but in strict mode throw that .ref or one of the others should be used?

Really don't like the idea of strict mode specific behavior. Maybe we should drop the observable prefix?:

@observable
@deepObservable
@shallowObservable

observableArray() // or just array
deepObservableArray() // or just deepArray

5. Rename observable.box to observable.ref?

No, because we still need observable.deepBox and observable.deepRef is really weird...

Additional suggestions:

1. replace strict mode by specific option forceActions (or something like that)

2. drop autoconversion support
   This is controversial, but if we would agree that a code that uses references and explicit conversions is far more easier to understand why not use it?
   Cons:
   Code may be a bit more verbose.
   Code may look a bit less fancy (because there is less magic!).
   People may forget to convert objects into observables (but currently they have to know that the object is cloned, which is not evident from the code and also that sometimes it isn't made observable at all, because it has a prototype etc)
   Pros:
   Less magic, code is easier to follow, things are easier to understand.
   Simplified API.
   Simplified codebase.

Also consider that users are supposed to modify existing observables instead of replacing them, therefore the autoconversion actually shouldn't occur that often!

[spion]

Nobody read my getting-started point 😢

But anyways if you change the default from deep to shallow, and then you read the "10 minute introduction to mobx", you just broke completedTodosCount. Now you have to explain what depth is in there, or go all Java like with class Todo {}

I think the whole premise that there is somehow a problem with defaulting to deep observables is completely wrong. I really don't see it. Its the exact right tool for a new user to get started with. Then as they learn they can be more explicit in what they mean.

There are problems with that approach, but have you considered the problems that the alternative will cause? I think they're even bigger.

There could be a documentation section "deep dive into ref/collection/deep" where the mechanics of each are explained (ref will watch assignments, but not content; collections will convert collections, but not individual elements; deep goes as deep as it can in doing the above)

[urugator]

Now you have to explain what depth is in there

I would say it's exactly the opposite, because there is no depth (at least by default). Anything which should be observable has to be made explicitely observable by user, not inside some setter behind his back, with additional side effects (cloning - lossing original reference, changing type or ignoring non-convertibles).
With @observable todos = observableArray() user sees that both - property and value are observable, which will probably raise the question "why" and may lead to noticing the difference between observable prop and observable value.

or go all Java like with class Todo {}

Why? you will just set/push observable(todo) instead of todo. And when you look at the code you see that inserted todo is not the original todo and that it's observable.
What's more, we may immediately throw an exception if todo is not convertible, so you won't be wondering why todo was not made observable by inserting it into observable prop/array...

@phiresky's question about inner modifiers also proves how unintuitive the autoconversion can be.

have you considered the problems that the alternative will cause?

I tried to outline some in previous post, but feel free to elaborate.

There could be a documentation section "deep dive into ref/collection/deep"

The thing is... should user need to read "deep dive section" to understand what his code actually does?

For clarification no autoconversion doesn't mean there won't be recursive factories, it only means that observables must be created explicitely (not by assigment/push/set etc).

[spion]

@urugator so its no longer transparent reactive programming. Its no longer enough to add decorators to your original class, you have to change the code too.

I think this will turn many people away. People went with angular due to the promise of the plain $scope object, and I think that a lot of the popularity of mobx comes from "plain objects", too.

Oh, and if that todo has some nested list, I assume you will also do observable(Object.assign(todo, {nestedList: observable(todo.nestedList)})) ?

Not to mention how this is all super error prone. If you forget to deeply apply observable on something, the result is that your UI wont update properly, and you might not even have any idea why! You'll have to chase down all dependant values and see if you're always replacing them in such a way that the replacement is observable. Hello new "stale UIs" reputation.

Sorry, I haven't changed my mind. The deep default (while somewhat magical and with some small caveats that are hard to understand) still has a better tradeoff value IMO.

[spion]

What's more, we may immediately throw an exception if todo is not convertible

Well why not do that then? If you are using the automatic @observable decorator, you have to ensure its on plain objects that only contain primitive and builtin values. Otherwise assignment will throw errors about unrecognized things.

[urugator]

transparent reactive programming

don't know what that means, but autoconversion seems very opaque to me (it's an invisible side effect of different operation)

Its no longer enough to add decorators to your original class, you have to change the code too.

Yes, as I've stated the code is more verbose, because there is less magic.

popularity of mobx comes from "plain objects", too.

Autoconversion creates a false illusion that you can work with original plain objects (or graphs), while it's not actually true (it was in v2 and only in case of objects and it was source of other problems too).
Plain objects are gone at the moment they're converted. The difference is that currently you don't see when and where the conversion actually happens, which can be confusing.

Oh, and if that todo has some nested list

Why would you set observable to non observable and then create an observable based on it? (Maybe you're missing {} as the first arg of Object.assign?)

// Create new todo from scratch (I suppose that making things observable when they're created should be the most common scenario)
observable({
  whatever: 5,
  nestedList: observable([]); // or just [] see following
});

// Create todo from existing object (recursive factory - previous post - last paragraph)
observable(todo, { recursive: true }); // could be default or syntax can differ, that's not the point  

super error prone

Yes user may forgot to make something observable and it will definitely happen. What seems positive to me is that it's basically the only mistake he can make, it's easy to explain and understand.
If todo isn't observable, it's always because you didn't make it observable during it's creation. Not because it was pushed to array, which was assigned to property, which wasn't decorated by @observable or because you're modifying original non-observable reference pushed into array or some other cryptic reason.

Well why not do that then? If you are using the automatic @observable decorator, you have to ensure its on plain objects that only contain primitive and builtin values.

I think it would make some scenarios unnecessary complicated (converting nested structures with some inherently uncovertible items). Single object is easy to handle, but "let it be if you can't" seems like more useful approach when converting whole object tree. But the idea of "safe conversions" is interesting. Maybe we could have something like mobx.warnOnNonconvertible(true)? Or make it default for devel env?

[spion]

If we are making breaking changes, I think throwing on unconvertables could be default when doing deep conversion.

If you want it to stop the conversion, nest an explicit @observable.ref or something like that.

Wouldn't this only break existing code that is already (subtly) broken?

[urugator]

@spion While it's possible to convert deeply nested structures, it's impossible to cofigure it's specific parts, so requiring this configuration (ref/shallow) seems impractical to me...

const state = observable(window.__STATE__);
state.something.somewhere.currentElement = Document.createElement("a"); // throws? not cool
// I would have to convert the whole tree by hand, because some part must be configured with ref/shallow...

[spion]

The idea is, since you are using a deep automatic observable, those work with plain, known objects and collections only. If you want something else, you use explicit annotations i.e. you use a ref-box to wrap the element. (something.somewhere.currentElement = box(elementVal))

Unlike the reverse idea, forgetting to use the proper wrapper throws rather than being incorrect and causing stale value bugs elsewhere.

[mweststrate]

Thanks for your input guys! I really like the discussion that is going on here. It really feels like nicely highlighting the cost and consequence of either direction

@spion yes I will definitely consider the impact on the 10 minute introduction :)

Also check #1321, I incorporated a few changes discussed here that I felt comfortable about (isObservableProp) already there.

[urugator]

explicit annotations

As presented, it's not always possibe to use annotation/modifier without changing the architecture and reimplementing the conversion by hand.

something.somewhere.currentElement = box(elementVal)

Not a solution, it introduces another unnecessary observable, but more importantly the nonconvertible can be present at the moment you passed it to observable. Another example:

const js = Mobx.toJS(original);
const state = observable(js); // throws
// currently the conversion rules for toJS and observable are the same but inverted

I think I understand where you're heading. You basically want to enforce the rules in way, that the side effects of autoconversion won't be noticable, so we could preserve the illusion of working with normal objects, while removing or minimalizing the possibility of getting into trouble.
I am convinced that it's not doable in bulletproof way, so we shouldn't even try and make this clear right away at the cost of slightly more verbose code and more responsibility passed to user.
While you're convinced that it's doable in a realiably enough fashion, so we should keep things seemingly trivial at the cost of higher overall complexity, which user has to explore only after he hits an actual problem.

[spion]

@urugator then I suggest we

• make a catalogue of summaries (with examples) of things that are problematic with deep observables,

• see which of those can be solved in a bullet proof way (assuming breaking behaviour like throwing on unconvertable objects, which is fine for .nextmajor)

• see what is left over

Then make the same list for the opposite approach similarly. This should result with enough info to make a good decision.

Off the top of my head... (will add examples later as they come)

deep problems

• unexpected mutation on assignment (what problem does it actually cause? assigning plain object to observable property forces plain object to become observable #644 doesn't explain what sort of debugging was necessary and why - if it did, the problem might be fixable by making the actual problem behaviour fully transparent as if the object has not actually been mutated, instead of respecting some platitudes about assignment)
• isObservable (fixed)
• unexpected non-conversions of unsupported objects (fixed by throwing)
• unexpected broken conversions of unsupported objects (does this happen?)

shallow problems

• forgetting to convert to observable will result with silently stale data
• more concepts to understand to even get started
• default no longer "plain objects with decorators" will be off-putting for many
• unreasonable insistance on making one of the 10+ methods in the API surface area be the default exported method (what is benefit to having one of them be the "default"? you still need to use the other 9)

[urugator]

@spion shallow/deep by default and no-autoconversion are two seperate discussions.
I will try to address all (deep/shallow) so far mentioned problems, but with no-autoconversion proposal:

unexpected mutation on assignment

I am not sure what you mean, because this is not the case since MobX 3.0, everything is cloned before converted.
With proxies, we have to use the proxy instead of the original object.
The actual problem is an unexpected loss of the original object reference and complicated conversion rules.
The loss of original reference is inevitable, but without autoconversion it's at least transparent and consistent:

const o = observable({ a: null });
const a = {};

// current behavior
o.a = a;
o.a !== a; // eh? (actual behavior depends on type of "a", whether "a" is already observable, on modifiers applied to "o.a" and where "o.a" comes from)

// without autoconversion
o.a = observable(a);
o.a !== a // obvious

forgetting to convert to observable will result with silently stale data

Solvable by:

1. Throwing on non-observables + nonObservable(value)

const o = observable({});
o.a = 5; // ok, primitives are ignored
o.a = observable({}) // ok, value is observable   
o.a = {}; // throws "Attempt to assign non-observable value to observable field, if this is an intention please wrap the value in Mobx.nonobservable(value)"
o.a = nonObservable({}); // ok, the intention is explicitely stated (it's like dangerouslySetInnerHTML), the value is unwraped during assigment    

1. (alternative) Throwing on non-observables + unsafe(operation)

const o = observable({});
o.a = {}; // throws
Mobx.unsafe(() => {
  o.a = {}; // ok
})

2. (not required, but could be more convinient) making observable factory recursive by default

const o = observable({ a: [{}] });
isObservable(o.a[0]); // true

const o = observable({ a: [{}] }, { recursive: false });
isObservable(o.a[0]); // false

unexpected non-conversions of unsupported objects
unexpected broken conversions of unsupported objects

Fixed by converter option passable to factories, defaults to "strict".

// default "strict" converter throws on non-convertibles
observable(new Something()) // throws
observable(new Something(), { recursive: false }); // throws
observable({ a: new Something() }) // throws
observable({ a: new Something() }, { recursive: false }); // ok

// "tolerant" converter leaves non-convertibles unconverted
observable({ a: new Something() }, { converter: "tolerant" }); // ok

// "forceful" converter treats non-convertibles as plain objects (is there a better term?)
observable({ a: new Something() }, { converter: "forceful" }); // ok
  
// own converter
const myConverter = any => any instanceof Date ? observableDate(any) : observable(any);
observable({ a: new Date() }, { converter: myConverter });

Originally I went for ignoreNonObservable option, but converter seems more powerful.

more concepts to understand to even get started

To get started user only needs observable and it's impossible to introduce staleness by default.

default no longer "plain objects with decorators" will be off-putting for many

It's not like that even now, only seemingly, causing a lot of issues.

unreasonable insistance on making one of the 10+ methods in the API

The whole API could look like this:

observable(value, { recursive: true, converter: "strict" }) // these are defaults

// "Modifiers"
nonObservable(value)
// or/and
unsafe(operation)

// Type specific factories 
object/array/map/set/box(value) // no options, non-recursive

// Decorators
@observable // no options, no modifiers, simply makes property observable (same as current @observable.ref)

[spion]

everything is cloned before converted.

I'm saying that may have been a premature "fix". What was the un-transparent behaviour of the modified object? It wasn't clear from the issue. Maybe it was possible to make the modification effectively invisible?

It's not like that even now, only seemingly, causing a lot of issues.

It is like that, for me, and its not causing any issues whatsoever. The only serious issue has been array concatenation, and i consider this a problem with the standard library guessing too much (and lodash et al following its example).

The whole API could look like this:

I count 7 or 8 methods there. Also, flags values count as additional methods as you have to learn the meaning of each. Close enough!

The marketing issue is real, by the way. I constantly sell mobx to people by saying: go to https://mobx.js.org/getting-started.html and see how learning 3 decorators gives you more than Redux ever did (and its true thanks to the superpowers of computeds).

[urugator]

Close enough!

Close to what? Is there a target?
You don't need to touch specific factories, they are mainly required for internal implementation or if you prefer type safety (they also shows whats convertible).
You don't need to know about recursive/converter/nonObservable/unsafe until you have specific needs or you hit "unconvertible error" (which indicates specific need).
So you just have to slap @observable/observable() everywhere, if you forget or when it's not possible, you get an error with a hint for solving the situation...

[mweststrate]

Thanks for the input so far guys!

Some personal conclusions so far

1. auto deep observability is a key feature for simple cases, quick onboarding and marketing
2. cloning is fine if clearly communicated; it is an (maybe not so) explicit process of making data part of your state. It is fine that the original data (e.g. server response, event) is not the actual state in the end
3. I think we should optimize for two cases:
   1. Pure plain object / array data tree. This keeps 10 minute intro and such as is
   2. Class based models offering fine grained control over observability

Proposed changes

Based on that, some proposals to sharpen the API awareness, difference between properties and values, ec, I feel quite confident on making these changes:

1. Separate isObservable and isObservableProp
2. observable(value): refuse unproxyable stuff (use observable.box)
3. @observable prop: refuse unproxyable, non-primitive, stuff (use observable.ref). This is not strictly needed, but I think it will help people to understand we don't convert classes
4. @observable.shallow: throw on anything except for: arrays, maps, null, undefined (see also don't throw exception when assign a primitive value to @observable.shallow? #1336). I am doubting whether it should support or throw on plain objects
5. @observable.deep: idem
6. Make converted plain objects non-extensible to avoid non observable key pitfall. If people want to add non-observable based properties to an object later on, the object should have been converted by means of extendObservable

A bit more radical changes which make the api more consistent but upgrading more complex

1. Make all observable.* factory methods shallow by default
2. Give them a { recursive: true } option as second argument to override the default. This is probably slightly less confusing of the shallow notion
3. Rename observable.object to observable.record. This communicates more clearly that the keyset is fixed, and also prepares the place for observable.object in MobX 5, for extensible objects :)
4. make @observable alias for @observable.ref instead of deep. (or kill it completely)

Summarized:

@observable - keep, but stricter, see above. Maybe make like `ref`
@observable.ref  - keep, but stricter, see above
@observable.deep  - keep, but stricter, see above (or `@observable.recursive`?)
@observable.shallow - keep, but stricter, see above

`observable(value)` - creates proper type of collection, with `recursive: true`
// below ones will become non-recursive by default
observable.box
observable.record - like current observable.object, but more explicit that keyset is limited
observable.array
observable.map
observable.object - reintroduce in next major; with proxies and dynamic key support
observable.set - introduce in next major

extendObservable - make shallow. 
extendDeepObservable - introduce?

Some remaining questions

1. call the "deep" concept deep or recursive?
2. Should we kill @observable.shallow x = [] and encourage readonly x = observable.array() instead? (and maybe same for @observable.deep then. But then @observable should be like @observable.ref?

Reduce the need for decorators

Decorators don't seem to stabilize in the standard, so I think @urugator 's proposal to introduce our own decorate method is really nice.

[urugator]

call the "deep" concept deep or recursive

I used term recursive because it applied to the process of making something observable and not to the resulting "observable type" (due to a missing autoconversion), also there wasn't any shallow counterpart (because not needed).

cloning is fine if clearly communicated

Imho it's impossible to communicate this clearly with autoconversion, because you can never tell whether you're assigning to something observable or not. This is because the "observability" is defined on absolutely different place than actually applied.
Therefore you're unable to tell, whether the assigned/pushed/set thing will be cloned and made observable. In simple cases the user usually doesn't even notice this, the more he's confused when something doesn't work, because he's absolutely unaware of this behavior.
This is NOT how plain objects normally behave, the argument that autoconversion allows the user to work with plain objects as he's used to is ridiculous and untrue.
Instead of making these things clear to the "onborading" user right away, we're pretending that these things don't exist for the sake of keeping the "getting-started" more attractive/sellable/less verbose/whatever, while additionaly complicating logic and api.

Without autoconversion, there would be 2 additional observable keywords in the whole getting-started:

@observable todos = observable([]); // here

this.todos.push(observable({ // here 
	task: task,
	completed: false,
	assignee: null
}));

That's all. 20 chars is the difference between "quick onboard + selling point" and "simpler api, simpler impl, zero confusion" (if we would use "getting-started" as metrics, which is silly imo)

throw on anything except for: arrays, maps, null, undefined

I don't understand the "strictness" of shallow/deep (especially deep). The shallow/deep is all about dynamic conversion (otherwise user would use specific type as suggested in "Some remaining questions" point 2), why it can convert ANYTHING (primitives,etc) INSIDE it, but not the passed value itself:

@observable.deep something;

@action setSomething(any) {
  // so I want to turn "any" into observable
  this.something = any; // nope throws on any
  // wtf workaround
  this.something = { temporal: any };
  this.something = this.something.temporal;
}

[spion]

@urugator I would personally probably never gotten started with MobX if I saw that every single record needed to be wrapped with some function.

Here are a couple of questions for you:

• @observable x = [] does this throw, or make an observable ref to a non-observable array creating staleness?

• What about arr.map(x => obesrvable({a: x})) ? Do we add observable there too? Do we throw or create non-observable objects if its forgotten? Where do we put the line? Will there even be line or every object stays plain by default?

I don't see the problem. Setters can ignore the reference being set and create their own, its a JS feature. Methods like push can ignore the ref being pushed and create their own.

Instead of helping, I see this creating incredible amounts of useless boilerplate with observable wrappers flying around everywhere.

I would be more interested in an experiment to bring mutation on the right-hand side back, then figure out why its not completely transparent to everything else.

[urugator]

@observable x = [] does this throw

Throws "Attempt to assign non-observable value to observable field, if this is an intention please wrap the value in Mobx.notObservable(value)" or something like that as already mentioned.

arr.map(x => ({a: x}))

Excellent example! Do you know what is the current behavior? Exactly! I am not sure either and I consider myself an advanced user! So I had to try it out... you may be quite suprised with the results:

console.log("deep");
const deep = Mobx.observable.array([{}, {}]);
let result = deep.map(x => ({a: x}));
console.log(Mobx.isObservable(result)); // false (array)
console.log(Mobx.isObservable(result[0])); // false (array's elements)
console.log(Mobx.isObservable(result[0].a)); // true (cloned original {})
// shallow
console.log("shallow");
const shallow = Mobx.observable.shallowArray([{}, {}]);
result = shallow.map(x => ({a: x}));
console.log(Mobx.isObservable(result)); // false (array)
console.log(Mobx.isObservable(result[0])); // false (array's elements)
console.log(Mobx.isObservable(result[0].a)); // false (original {} ref)

It doesn't convert! Is it a bug? Is it an intention? I don't think it matters, it shows how twisted, unpredictable, non-trasparent the autoconversion idea is ... emotions and exclamations aside:
Array.map creates new array, so it's quite unlikely that it will represent state until you assign it somewhere (the state would be temporal otherwise).
When you assign it somewhere, that's when autoconversion kicks in (and also where you loose your references etc) and you're stateful again ... so without autoconversion the situation is very similar:

this.foo = arr.map(x => ({a: x})); // throws to avoid staleness
// =>
this.foo = observable(arr.map(x => ({a: x}))); // no need to make individual things observable before they actually became part of state again

Consider that .slice() also creates non-observable copy. Performing autoconversion on returned array is probably unnecessary in most cases (nobody complained?) and could get you to opposite extreme, where you have to call toJS after every operation (both accompanied by some pefromance loss).

Setters can ignore the reference being

Sure, but don't call these "plain objects/array" if they don't work like that.

[spion]

Your unpredictability argument is my transparency argument. I read it as

Transparent behaviour kicks in when you make an assigment, and since its transparent, the array.map issue hasn't come up yet. Thats a great result, its exactly the ideal goal.

Regarding plain objects and arrays, Honestly, I'm not happy about the cloning change, but I can live with some impedance mismatch so long as its possible to write code that results with the same result with or without decorators.

I will acknowledge that at this point there are no serious technical flaws in your proposal that I can see. My only remaining concerns are aesthetic - a strong preference for errors where the software failed to convert/understand something over errors instructing the human to do something that the computer could've done by itself; a strong dislike of verbose noise in code.

edit: oh, and whether its possible to set up all the error / wrapping / conversion cases in a way that is consistent and makes sense.

p.p.s. I still find this.array = this.array.map(...) throwing an error to be very, very confusing.

[urugator]

Your unpredictability argument is my transparency argument.

But you don't know for sure whether the conversion actually kicks in, it doesn't work like this everytime, that's why it's not transparent (and also because it tries to look like a simple assigment while it is not). As stated earlier, the behavior depends on configuration, which is placed on some absolutely urelated place.
In my code snippet you see that the array is explicitely created as shallow/deep, but in real code you don't know from where the array comes from, how it was created or to which property it has been assigned, you don't know what is or isn't causing the conversion...
With explicit conversion, it always works the same way, there're no doubts about the behavior.

over errors instructing the human to do something that the computer could've done by itself

But that's the thing, the computer can't do it itself or to be precise JS can't. The situation would be quite different if it would be technically possible to "enhance" existing objects. Actually it was partially the case before Mobx V3 - the objects were converted in-place, while "arrays" were considered a special case (due to a technical limitations).
It's not noise, it's necessity or rather sane compromise under the given circumstances.

[spion]

In my code snippet you see that the array is explicitely created as shallow/deep, but in real code you don't know from where the array comes from, how it was created or to which property it has been assigned, you don't know what is or isn't causing the conversion...

Sounds to me like the solution is to control observability by type, not by modifiers e.g. you should be able to declare that "immutablejs objects are always considered refs".

Then you can autoconvert built-ins, not autoconvert objects and classes that have at least one observable decorator, treat above known types as specified and throw on everything else. The "shallow" concept also goes away.

To be honest, I'm not worried so much about the "assign value to observable, modify that value and expect reactions to run" scenario. I think "expect observable value to also change" is sufficient compromise in this case, and you can get that with proxies (since they're not clones). Mostly when this usually happens in the wild, you are initialising an observable property with a new value coming from a scoped variable that goes away soon, so the observable property is already dirty within that action.

[vonovak]

Just came across this and remembered observer from mobx-react. Observer uses autorun under the hood, and my only idea here is that observer could be a name that shows the relation to autorun more than it does now. Just wanted to share this, but I do think that observer is actually a pretty good name!

[MastroLindus]

I come late to the discussion and I couldn't read everything, but these are just my 2 cents:

I am looking forward to a new version for big features like proxy support, support for native sets, and so on, not really for API changes.

The API can always be improved, but in its current state is not in a bad shape at all, and the confusion it can generate usually goes away pretty fast after a little experience with it.
Moreover the risk is to generate as much confusion to the people already accustomed to the way mobx work, even worse if there are many breaking changes to deal with.
(e.g.
"observable(value): refuse unproxyable stuff (use observable.box)
@observable prop: refuse unproxyable, non-primitive, stuff (use observable.ref). This is not strictly needed, but I think it will help people to understand we don't convert classes"

these two alone will need to be fixed in many places in our codebase)

I understand that the API needs to get reviewed from time to time to not become obsolete, but there were already quite some changes in version 3, I wouldn't mind waiting some future versions ( > 4 )for the reworkings proposed here.

Regarding the changes itself, one compromise would be to keep the current API but add tslint rules (or equivalent for flow/js) that would generate warnings when used in the improper way (for example when using @observable with a non-primitive ). The outcome would be to preserve the simplicity, non-verbosity of the current API but still help newcomers to understand what's going on and avoid mistakes.

so tl;dr;: split the feature changes from the api changes (or postpone them directly, and switch to lint warnings)

[urugator]

@MastroLindus Don't you think that the need to use some additional tools (flow/lint) is yet another barrier to overcome before getting started?

[MastroLindus]

@urugator I see your point, and you might be right, however:
1)A growing part of the js community is embracing compilers, transpilers, bundlers and I think (or hope) that is just a matter of time before linters are also considered necessary.
Nobody likes to have a lot of tools to do the job, me neither, but the value to catch up errors (or in case of linters, potential errors) earlier is invaluable for any useful project, especially built in large teams.
Moreover it would still be an "opt-in", people could still get started without as always

2)I tend to agree that a clear, understandable API shouldn't be replaced simply by some external tools, but as with many other APIs for complex functionality, the sweet spot between being concise and verbose, deceivingly simple or overwhelmingly complex is really hard to find.
I feel like that the current API is really close to it, and I am afraid that changing too much could easily do more damage than good, therefore the idea to postpone it for now and help the new users in other ways (e.g. tools).

Given that, many arguments in this thread make sense, and if the API changes, I will simply roll with it :)

[urugator]

A growing part of the js community is embracing compilers, transpilers, bundlers and I think (or hope) that is just a matter of time before linters are also considered necessary.

I hope not, this kind of reasoning actually frightens me tbh:
Whats the point of programming language if it can't be undestood by human without linter?
Whats the point of scripting language if it has to be compiled?
Whats the point of standard/common language if it has to be transpiled?

These tools are workarounds, they are used due to a technical limitations or specific needs, people shouldn't use them, design things around them or rely on them, unless it makes their product cheaper.
Once we consider these as necessities, there won't be any benefit in using JS.

But I think you made a good point that radical or frequent API changes can undermine user's confidence in Mobx.

[MastroLindus]

In short, I agree tht transpilers are used because of technical reasons(even if they do provide quite some benefits), but linters and ESPECIALLY compilers are immensely useful since humans can be as good as you want but in specific tasks (identify errors, bugs, keeping order during huge refactorings etc) they will never be as exact and precise as programs designed just to do that. They lower the cognitive effort that a programmer (and especially a team of programmers together) need to use in order to produce, verify, and maintain reliable code. And I guess that going forward is inevitable that tools&software will do more and more of what before was done manually.

Anyway, I think we got a bit too far in this, and I apologize for that.

I would really happy to continue discussing those points with you elsewhere, as I am afraid it would be off topic here and not really useful for mobx :)

[AoDev]

Hello,
I am very happy with the current mobx API. I am an early adopter and I could say I have become some kind of evangelist in the past years. Especially when building React apps. The reason is its simplicity and it's partly because it has already a quite good API.

I would rather improve the docs. Focus them more on use cases and investigate what people ask themselves when they start using mobx. It's hard to answer your original question for people who are already seasoned mobx developers, because we aren't "surprised" anymore.

Well yes, one thing that gets me from time to time. If I forget to use observer on a component, it will still update in dev. But when I make the production build, it will stop reacting without warning. :D

[AoDev]

Thinking while working. I do have something in mind now.

Could we do something about the need to wrap callbacks in "async actions"?

I know there are babel plugins but, it would be nice to have something like this directly in the API. People wouldn't have to wonder why using runInAction, action(() => {}), etc...

[urugator]

@AoDev are you aware of asyncAction? (it's mentioned at the end of the docs...)
I have already proposed to move it from utils to mobx package...

[AoDev]

@urugator I was not aware of it, thanks for the link. :)

But it seems to work only with generators.

While it works, it's still another variant of "how to write async actions because you can't just use action". I would rephrase my question to the following: is it possible to have a simple decorator that would work with any syntax like promises or async, or is it just technically not possible?

[spion]

Last time I glanced at things, its not technically possible due to bad interactions in the design decisions of promises and async/await.

[urugator]

Well the syntax offers the same comfort as async/wait and interoperability with promises. You just have to replace async with * and await with yield:

async method() {
  const a = await fetch(this.url);   
  return await this.asyncAction();  // obviously you don't have to await/yield here
}
// ==>
@asyncAction
*method() {
  const a = yield fetch(this.url);   
  return yield this.asyncAction();
}

We could also come up with a function which wraps standard promise and returns own "thenable":

return Mobx.Promise(fetch(url))
.then(data => {
  /* runs in action*/
  return fetch(url); // wraps the result
)
.then(result => { /* runs in action*/ }) 

[spion]

Afaik a custom promise wont solve the issue, because of two issues with the design of ES6 promises and async/await:

• async/await converts all thenables to built-in promises
• the conversion forces execution after the thenable's callback.

For a demo, try this code:

async function test(thenable) { 
  console.log("af: before await"); 
  await thenable; 
  console.log("af: after await"); 
  return 1 
}

test({
  then: function(callback) { 
    console.log("Calling callback with value"); 
    setTimeout(() => { 
      callback(2); 
      console.log("Called callback with value"); 
    } , 200); }})

The output is:

af: before await
Calling callback with value
Called callback with value
af: after await

i.e. its impossible to "trap" the code after the await in, say, action(), by providing a different thenable.

[urugator]

I wasn't suggesting to use that custom promise inside async ...obviously you can await it but the code inbetween awaits won't run inside action... (the code in promise should..)

[mweststrate]

asyncActions will be exposed as flow in MobX 4 in the core package.

Beyond that, closing this issue, as the new api can now be beta tested!

npm install [email protected]

Migration guide: https://github.com/mobxjs/mobx/wiki/Migrating-from-mobx-3-to-mobx-4

[mweststrate]

Thanks for all the input! And if things seems to be trouble some feel free to reopen. The discussion kinda took place into two different issues, so I didn't respond to all issues, but rest assured I did read all of them and used them as input for the new api design :)