Closed mweststrate closed 7 years ago
@andykog @urugator @capaj @Strate @mattruby @donaldpipowitch @kenotron Very interested in your thoughts about the above :)
This is great! I didn't know about this automagic conversion in past and I had to spend a lot of time investigating what was wrong with my code, ending with forcing object back to plain with a toJS
method.
What would happen if you do observable
on plain object, it became observable only at first level, or deep level too?
only first level
Sounds good, but maybe it would be nice to have observableDeep
helper (maybe not), I am really don't know :)
probably it should make anything observable that is initially in there indeed
@mweststrate
asFlat
as in the current api, then to force explicitness. I think it is such a common usecase where properties of an object inside an observable array are mutated, that it would be wrong to default into plain non-observable objects.3.this certainly brings MobX closer to the API we'll have in couple of years with ES6 Proxies. With proxies you won't modify the source object, but you instead generate a proxy object and use that instead of the original. It is a good idea in that regard to leave the source untouched.
calling observable on an already observable thing should probably warn
warn? Why not go a step further and throw an error? This would make sense if it is perf expensive. I don't think it is because it just checks one property right? If it is indeed perf cheap, I'd probably don't bother throwing/warning. If someone does that a lot, it is not nice, but it won't be a problem.
+1 for observableDeep. Other than that, I do think it's clearer this way. Many people don't realize how things are working today.
On Nov 12, 2016 5:55 AM, "Artur Eshenbrener" notifications@github.com wrote:
Sounds good, but maybe it would be nice to have observableDeep helper (maybe not), I am really don't know :)
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/649#issuecomment-260117909, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIrciAK-Tr2KIcCz151zB0i2cw193BNks5q9akugaJpZM4KwYbt .
@mattruby
Many people don't realize how things are working today.
Agree, but should we change the API because people don't read the docs? Maybe we can just make it more visible somehow?
Or have a shallowObservable option? I could go either way. That magic would still be available with deep. Just more explicit.
On Nov 12, 2016 7:49 AM, "Jiri Spac" notifications@github.com wrote:
@mattruby https://github.com/mattruby
Many people don't realize how things are working today.
Agree, but should we change the API because people don't read the docs? Maybe we can just make it more visible somehow?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/649#issuecomment-260123171, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIrcr08CGvMuvY2MjAwcpHMUSUlwJyxks5q9cP8gaJpZM4KwYbt .
deepObservable
, asFlat
/asStructure
can still be useful, but not very important.should one need to write
@observable todos = observable([])
or should@observable todos = []
suffice (and convert[]
) still automatically?
I think class { @observable todos = val; }
should give the same result as class { constructor() { extendObservable(this, { todos: val }); } }
, so rather 1st option.
@andykog I like the idea of deepObservable
. If I understand correctly if used on an array. it would behave the same as current API? I'd definitely can get on board with that! It would still allow for the same simple workflow if explicitly stated on initialization, while observable
would be just flat observable.
@capaj, that's @Strate's proposition, I think it should work just the same way as current observable
: recursively apply to all the object props, array items…
Basically repeating ideas from #211
const Mobx = require("mobx");
Mobx.Observable.shallow(obj, modifiers) // no default conversions, same as asFlat
Mobx.Observable.deep(obj, modifiers) // same as current observable
Mobx.Observable.computed(obj, modifiers)
Mobx.Observable.extend(obj, propDefinitions) // no default conversions, modifies passed obj
Remove observable
- name is too vague, provides no hints about existence of other options, function tries to solve too much, logic is hard to predict and understand. Functionallity is replaced by Mobx.Observable.deep(obj)
or Mobx.Observable.from(obj, [Mobx.Modifier.deep()])
Remove asReference
- it should be default behavior.
Introduce deep
modifier/decorator.
should the whole api be more more explicit: @observable todos = observableArray()
Yes, but move the explicitness to the left side (decorator/modifier) so the stickyness is apparent @Mobx.Decorator.deep todos = []
or @Mobx.Decorator.observable(Mobx.Modifier.deep()) todos = []
I think that better naming alone would solve some issues - these changes are in fact just cosmetics, they don't add or remove features. The thing is that current API doesn't provide any hints about what's going to happen (aside from vague "it makes thing observable") or if there are other options available. As a result, users discover these things by making mistakes, which is not ideal.
EDIT: removed Observable.from
and replaced by Observable.shallow
(does actually the same)
Hmm.. I'm not sure about observable
should clone instead of enhance. Although modifying RHS is confusing, storing a clone is confusing as well:
This was confusing:
const todo = { done: false}
isObservable(todo) // false
const todos = observable([]) // observable array
todos.push(todo)
isObservable(todo) // true, confusing
But this is confusing as well!
const todo = { done: false}
const todos = deepObservable([])
todos[0] = todo // silently being cloned and converted to observable object
todos[0] === todo // false, it's cloned!
In that sense, I like @capaj's notion that Proxies allow this to be transparent, and moving in that direction would encourage enhancing.
@mweststrate, I thought deepObservable
should only create create nested observable structure, without turning on automatic conversion. If so, your second example will loose its meaning.
Ok, two proposals. Not too many comments, as the API should be self-explanatory
//// Non-decorators
deepObservable(object | array )
shallowObservable(object | array)
observableMap(object)
observable(any) // creates a boxed observable
//// Decorators
// shallow observable decorator forbids reassignments (or assignments of non-obsrvables?)
// to avoid needing to convert assigned values (e.g. a fture `obj.x = []`)
@shallowObservable x = []
// .. has only deep has magic conversion behavior, works as current `@observable`
@deepObservable = {}
// reference only!
@observable = {}
// as is
@computed
Based on @urugators proposal. Just not sure if from
is fit; as that suggests an uniform target structure (like Rx streams), which isn't the case in MobX?
// Shallow:
Observable.object({})
Observable.array([])
Observable.map({})
// Boxed:
Observable.value(anything) // or .reference / .ref / .boxed / .atom / .cell
// Behaves as current `observable`
Observable.deep(array | object)
//// Decorators
@observable prop = value // reference observable
// encouraged form, no reassignments allowed, mutate array instead
@final todos = Observable.array([])
// disencouraged form, both modifying and reassigning `todos` is allowed
@observable todos = Observable.array([])
Note that the second proposal introduces @final
. This might look counterintuitive, but might actually avoid confusion between the next two examples by simply forbidding the first:
// will RHS become observable? or will it stay plain array and `user.tags.length` not be observable in the future?
user.tags = ["donald", "duck"]
// unambigous; tags are replaced, values are copied over. tags stay observable. Original array is untouched
user.tags.replace(["donald", "duck"])
// the latter pattern can be encouraged by doing:
class User {
@final tags = Observable.array()
}
Didn't add asStructure
to the above examples.
Are people actually using that? Maybe it should be part of mobx-utils or something. As modifier doesn't add much value imho, except if combined with computed
? Do people actually do that?
2.seems like a bigger change.
I kinda like MobX as it is now and I am a conservative developer when it comes to the tools I hold dear. So for me, 1. seems like a better choice. I am not thrilled by how long the new names are: shallowObservable
for example is gonna be a little tiresome to write without autocomplete.
@andykog yeah I have no clear opinion about that yet, I can imagine people might have libraries / architectures relying on that behavior, so it feels it should remain available somehow? Anyway, I got stuck on that with the following:
const store = deepObservable({ todos: [ { completed: true } ]})
isObservable(store.todos[0], "completed") // true
store.todos.push({completed: false})
isObservable(store.todos[1], "completed") // ... ?
// what is expected now?
// 1. true, automagic conversion of original
// 2. true, automagic conversion in clone
// 3. false. Confusing, first object observable, second not?
// 4. throw / warn somehow you probably wanted to add an observable
@capaj can you check the numbering in your answer ;-) seems off
@mweststrate should be fixed now on github. It is probably wrong in the email notification. I should start previewing before posting 🤕
Sounds good! I usually use asFlat
for arrays anyway.
Could observable()
take another argument with what to transform added inner elements by?
observable(any, innerTransform = (a) => a)
So to do a deep observable, you could write:
const todos = observable([], observable)
todos.push({ completed: false })
isObservable(todos[0]) // true
I do believe that libraries should always evolve if necessary, but I am against all of these changes, except number 3 (which I don't think matters much anyway).
It's really not hard to avoid these problems if you read the docs. Taking the Observable API from 1 to 4 decorators (Proposal 1) or bringing in a bunch of helper functions like Immutable (Proposal 2) really increase the barrier of entry to this library.
One of the things that really sold me on MobX is the simplicity of "it just works". And it REALLY DOES "just work". And when it doesn't work, I read the docs, and there are sane solutions to my problems.
As for @computed({asStructure: true})
, yes I do use it and it has really saved my ass in my latest project.
This library has an amazing and easy to learn API and I'd hate to see that change.
Thinking about it, having observable arrays make future values observable tracks the behavior of observables objects. For example
class Foo{
@observable obj = null
}
let f = new Foo();
If you set f.obj to an object literal, then everything therein will be made observable. If, however, you set it to an instance of a class (or really anything with a prototype) then this is not done, since the assumption is that you will have already handled making what needs to be observable, observable (in your class definition, presumably).
Arrays behave the same way. If you add a class instance, it's left alone. If you add an object literal, it's made observable.
I'm not sure breaking this symmetry would accomplish you much.
I agree with @arackaf. I have all my state as an obeservable and have no illusion about what is and isn't observable and if its portions should or shouldn't be observable. I trust in the dependency graph you generate to do its work, and it hasn't let me down once :rose:
Edited my original post (removed from
)
To Proposal 2:
Observable.deep(array | object)
should be Observable.deep(anything)
Autoconversion(stickyness) on property could work like this:
@deep prop = []; // left side, sticky, autoconvert
@observable prop = Observable.deep([]); // right side, non-sticky, don't autoconvert
// Sticky shallows
@shallow prop = []; // = {}; // = new Map();
I know this thread was probably created because maintainers keep having to help people through edges cases. Early on @mweststrate helped me through the problem of dynamically added object keys are not observable (thank you, btw). But I use Mobx quite a bit and that's the only edge case I've personally ran into. There is a proposal for dealing with dynamic keys here https://github.com/mobxjs/mobx/issues/652. But adding to the available number of functions has overhead for the user (and the maintainers). I love that @observable
just works for most cases. That's the kind of magic I want.
This may be an unpopular idea, but just throwing it out there.
Would counting on having ES6 proxies available allow us to have a more streamline API with fewer edge cases? I know it would help when adding keys to observable objects, but my impression is it could help in other ways. It seems like it could at least allow for amazing DX with better warnings.
This chart has changed quite a bit since I last looked about 6 months ago: https://kangax.github.io/compat-table/es6/. Proxies are available in the latest major versions of desktop browsers, Node 6+, Electron, and iOS10. The latest Android browser isn't supported - not sure where mobile Chrome is on that list. That support covers a huge set of use cases. My point is that the current Mobx release is great and it can still be used in cases that require broader support. But for people who can depend on proxies, what could we do?
From my perspective, the proposals above seem to lose convenience or expand the API with too many options (and unfortunately I don't have any better ideas). API design can make or break a library and I'm concerned about where this could go. I don't know the internals of Mobx so it's hard for me to say how much proxies could help. Thoughts?
Would counting on having ES6 proxies available allow us to have a more streamline API with fewer edge cases?
Even though proxies might help with overcoming some langauge limitations, I believe they are mainly an implementation detail. Proxies won't change the fact that things can be observable in multiple ways and won't remove the need for defining observability. They might also introduce another problems, but we will never know until someone come up with an implementation.
proposals above seem to lose convenience or expand the API with too many options
Aside from proposed @final
, API is not expanded in any way, all the API is already here in form of modifiers or utilities etc. The convinience of current observable
is also preserved, it just have a name, which actually describes this conviniency.
I think thats part of the problem, we wish things to be simple, pretending they are not here, calling them edge cases, trying to escape from their complexity. And instead of enlightening them at least a little, we are trying to bury them, to move them from sight, to hide them behind a single magic function, so we can keep that false feeling that they don't exist.
There should be a mobx version that can logs where in the current code base automatic conversion is applied, so that before moving to the next mobx version these cases can be updated.
I think it is good to log deprecation warnings in development builds. I like Embers philosophy here: do not introduce new features with a new major version. Only remove old and deprecated code. Every new feature is introduced with a minor version. I know this is hard, but it's nice, if this can be achieved.
About 3)
const todo1 = { done: false }
const todo2 = observable(todo1)
todo1 === todo2 // true
const todos1 = []
const todos2 = observable(todos1)
todos1 === todos2 // false
I think it is good, if small hurdles like this could be solved in the future - if they can be solved without introducing more hurdles.
I think there are two worlds colliding:
As someone who fits in the second category, I'd love to remove all automagic conversions to observables, but I can understand why people using plain objects like this automagicallity. Maybe there is room for something like mobx/explicit
where the default configuration for decorators would be the same as using asFlat
for arrays and asReference
for rest?
Maybe there is room for something like mobx/explicit where the default configuration for decorators would be the same as using asFlat for arrays and asReference for rest?
I think that would complicate things even further. You would end up with two same looking pices of code doing two different things, based on some external circumstances. It would also be harder to combine the approaches. I usually starts with plain object and when it becomes too complex I replace it with class, that's quite common workflow I would say.
@urugator et al
This comment is apt:
Proxies won't change the fact that things can be observable in multiple ways and won't remove the need for defining observability.
You're right, that is true. I still think there is an opportunity to make this API and DX better if we assumed a modern environment that can run things like proxies. Here are some more specific thoughts:
dynamicObject
proposal #652 that would be unnecessary. We could also get rid of extendObservable
.asMap
could be replaced with native Maps. Something like @observable xyz = new Map()
. Similarly for native Set
s. If the developer is comfortable using and targeting new native data structures like this, Mobx could make it observable. asMap
has it's own set of methods that are like the native API but not quite. Going this route we could get rid of asMap
Back to the original comment about different kinds of observable behavior and the need to define it, I still think we can have a single observable
function that observes everything deeply. Shallow observables and references could be special cases (shallowObservable
and asReference
respectively).
If people aren't interested the next version of Mobx that targets modern environments, couldn't we still count on a modern environment during development? Developers using modern Chrome, FF or SF could get better warnings and errors using proxies. That could offload a lot of support questions to the code. They could then be stripped out during production.
I'm not attached to these ideas but I think it would be interesting to explore them.
Adding keys to an observable object would make them observable.
There are cases where it's undesirable.
We could also get rid of extendObservable
How do I make classes observable? I need to somehow define which properties and in which way they should be proxified. The extendObservable
will possibly change into something like proxify
, but it can't be removed.
map/set/array
Only difference between proxied built-ins and proprietary classes is that we can preserve the original object reference (including it's type) and maybe performance.
Notice that the initialy proposed "cloning idea" is actually agains preserving the original object reference, so we're not even sure if it's desired behavior.
You still need to turn these structures into observables (to proxify them). If it's done by observable(new Map())
or by asMap({})
has nothing to do with proxies and can be implemented now (and probably should).
I still think we can have a single observable function that observes everything deeply.
Nobody is trying to remove this function, it's usefull and it's used very commonly (by me as well). The proposals are just to rename it so that it's more apparent what it does and so there is a place to introduce explicit alternatives, which are currently implemented as modifiers of autoconversion rules.
couldn't we still count on a modern environment during development?
Opt-in devel utils/tools using modern features are fine, but different implementation of observables based on environment seems dangerous and hard to maintain. I don't want to see bugs in production, which are not present in development, because the implementation is different. The devel env should be as close to production as possible.
@urugator
Interesting - I have never made classes observable. I make properties within them observable on a case-by-case basis (so I also don't need asReference
). But I see your point and didn't think of that.
As for native Map and Set, I included them in that discussion because we were in the context of a modern browser where they could be relied on. But you're right we could (and maybe should) make those observable now independent of proxy support. If developers are confident Map
and Set
are supported in their environment, they can use them. If not, they don't.
Great warning about ensuring development tools remaining consistent between dev and prod. That has to be an important part of the discussion if folks start exploring that approach.
Interesting - I have never made classes observable. I make properties within them observable on a case-by-case basis (so I also don't need asReference).
It's not possible to "make class observable", only it's properties - but it's done by extendObservable
internally and it's the only option if you don't use decorators.
asReference
does not turn off the observability of property, but prevents it's value from being converted into observable. Maybe that wasn't clear before?
@urugator Thanks for the clarification. I thought when you said this:
How do I make classes observable?
You meant that you were doing something like this:
@observable class Store {
}
But I think I understand what you were saying. Maybe I use Mobx in a very narrow way (which is why I'm not running into many problems), which is to use observable decorators inside classes like this:
class Store {
@observable myString = 'a string'
@observable myNumber = 10
@observable myArray = [10, 100]
@observable myObject = {a: 10}
myUnobservableObject = {b: 100}
}
At this point I'm not sure if this is is helpful to clarify this thread or not. If not I apologize to folks.
Thanks for all the comments and feedback so far! This is super useful
So far two conclusions
observable
. Especially with classes it combines wellMy current ideas for the api:
A next proposal (this is quite an extensive, but also regular api):
// as-is
observable
extendObservable
// create boxed observable, same as observableRef, but only primitives acceptable
const number = observableValue(3)
// creates boxed observably
const selection = observableRef(null)
// creates shallow, observable *clone* of the given object
// won't recurse, won't make values assigned in the future reactive
const newTodo = observableObject({
completed: false,
author: userStore.currentUser
})
// similar to observableObject, but for arrays. Can be invoked without args.
observableArray()
// similar to observableObject, but for maps, can be invoked without args
observableMap()
// converts a tree of data recursively to shallow observables. This is quite similar to `observable(tree)`
// but it won't make new objects assigned in the future to tree automatically observable
const todoStore = observableTree({
user: { name: "Michel"},
todos: [{ title: "Make coffee", completed: false }]
})
// alias for observable, mainly to make more clear how observable relates to the other methods
autoObservableTree
// Similar to extendObservable, but like observableObject will only introduce shallow props
shallowExtendObservable
//// DECORATORS
// should be initialized with an non-empty object, array or map. Will convert that to a shallow observable
// doesn't allow reassignments (.replace etc should be used instead)
@shallowObservable
// as-is creats an auto observable
@observable
// replaces current `@observable = asReference(value)`
@observableValue
@observableRef
I don't understand const selection = observableRef(null)
, "reference" imho makes sense only in the context of object properties (or can asReference be used effectively somewhere else?).
I think the naming is quite misleading. One would expect that observable(obj)
is just a type neutral alternative for observableRef
/observableValue
/observableObject
/observableMap
, calling these methods under the hood, but that's not the case.
You know, if observable
makes objects deeply observable, wouldn't you expect that observableArray
do so as well? I would.
observableTree
... so it's basically non-sticky observable
?..
I just fear it will shows up, that there is so many possible combinations, that it won't be possible to cover each by it's own function and some more generic approach will be needed.
You know, for example, what about autoAsReferenceObservableTree
or shallowReassignableObservableStructTree
etc...? To be honest autoObservableTree
sounds already a bit ridiculous (the name). As for myself, I will probably never use observableTree
.
Btw, I think we should reconsider that cloning idea. Even though it seems right to not modify the client object in any way, it's mainly caused by limitation of current implementation (which has to modify an object), but not necessarily by a bad design decision. Would mobx clone objects, if they would be proxied? I don't think so. What's more, the argument about modifing the right side of the assigment (#644) is not valid. First of all, it's not possible to modify the right side of the assigment in JS at all. You're either assigning a primitive value, in that case the value is copied, otherwise you're assigning a pointer and the value of pointer (address) can't be changed either. Secondly there is literally no reason to expect that an object passed by reference somewhere (even by assigment), won't be modified in any way. If user want to ensure something like that, he has to use immutables/freeze/seal/etc or pass a copy, but it's his reponsibility, because he is the one who decided to pass an actual reference. The biggest problem I see, if mobx will always automaticaly create a copy, there is no way back, without providing additional API and complicating things even more. Currently, that's not an issues, because I can always copy objects myself without much of an afford and pass a copy myself. So I am against the cloning idea, unless someone will present some real benefits, as "users may not expect" or "it seems right" are quite poor arguments for such change (some users may easily expect otherwise).
Maybe we're trying to solve too many things at once. There are multiple quite seperate problems we are trying to deal with: recursiveness, stickiness, autoconversion, cloning, .... Maybe we should first find clear answer to each and then try to find out how they work together.
@urugator thanks for the comments! I completely agree it some doesn't complete feel right. There are a few questions to answer
asFlat
or not. I can imagine that often assigning a plain object and converting that to an observable is regularly a bridge too far; the object might be from an external lib, and needed to be considered a 'closed box' or 'struct', just like class instances.Reference btw basically means a by-reference-value; just a boxed reference:
const selection = observable(null) // create boxed observable
selection.set(todoFromSomewhere) // will automatically convert todo to an observable
const selection = observableRef(null) // create boxed observable
selection.set(todoFromSomewhere) // will just store the reference, but leave it's target untouched
The observableTree
was basically for the scenario where you don't want automatic conversion of any value assigned, but still want to convert a large chunk of data at once to an observable, otherwise you would need to do things like below. The main use case for this is dehydrating stores quickly if you have stores based on plain observable objects
// create observable array
// with as initial contents, the original contents converted to observable objects
// this will become cumbersome for a big tree
const observableTodos = shallowObservableArray(todoJson.map(todo => shallowObservableObject(todo)))
Reference btw basically means a by-reference-value; just a boxed reference:
Oh, I understand now. I quess it's mainly intended as decorator/"modifier", as the following seems like a more natural solution to a possible problem:
const selection = observable({ value: asReference(null); })
while using selection.value
instead of selection.set()/get()
,
So, from the usage perspective, it's new decorator for ES Next users, for others it's basically renamed asReference
modifier.
2 auto conversion of assigned values. Or asFlat or not.
I think these two are different. The asFlat
is an opposite to recursiveness, while autoconversion is an opposite to asReference
.
3 stickiness
I would like to explain, why stickiness is important and useful for me personally. I will be talking mainly about classes, but I think it applies to plain objects as well.
One usually wants to convert the plain objects/collections inside a class, not passing already observable objects created outside and forcing the outside to depend on mobx.
The most clear, explicit and readable way of doing so, would be to do the conversions inside a setters.
But as you can imagine that would get annonying quite fast, especially when trying things or prototyping. So to me, stickiness is in fact a convinient setter, which converts a plain structure to a structure needed internally by class. With that in mind, if shallowObservable
will prevent from reassigning the value, the stickiness really looses it's purpose in this case. And I don't think it shoudl. When my class needs to hold a shallowly observable array and I want to provide an API for settings the whole array, I will do it anyway, but it will require more writing, which I am not sure is justified.
Wasn't prohibition of reassigment introduced only because of cloning? I am getting lost a little bit :)
EDIT: Question: Should the stickiness be configurable?
Btw it's just a detail, but I don't like the need for switching the "observable" word position in function names (observableArray
, shallowObservable
), that's one of the reasons I prefer simply mobx.array
,mobx.shallow
or Observable.array
I just want to point out that the automagic observable is mainly useful because it doesn't happen to class instances. I think the current state of affairs is good since plain objects are mainly used to group different observables together and you want the grouping itself to be observable too - so if I understand correctly I'm -1 on proposal #1.
I'd like to see modifiers killed anyway since #1 is easy to opt out of by naming your things.
I'm partial to #3, when I assign it with the decorator I expect the observable to enhance and not replace the object. Again, this is probably irrelevant for classes so it's not a huge issue anyway.
I think that if we encourage people to use classes most of these are non-issues anyway, and that we shouldn't cater to everyone everywhere. These are all cases that I rarely ran into.
My only wish is that the API surface gets smaller and not bigger - it's too big anyway.
Just a note about extending existing object instead of creating new one. What happens, if you would like to migrate to ES Proxies? Proxy is a new object, it could not be enhancer of existing object.
@Strate no other way around, a Proxy just traps an existing object, but the original object is the one you keep distributing. Proxy-ing is intended to be transparent, which makes it a very nice fit for MobX
@mweststrate, you have to distribute a Proxy instance instead of original object if you want to detect changes and properties lookups:
var original = { };
var proxied = new Proxy(original, { set: function(target, name) { console.log(`set ${name}`) } });
original.a = 5; // nothing
proxied.b = 5; // prints `set b`
That behaviour is opposite to current mobx's enhancing strategy.
@andykog @Strate oops totally missed that. Feeling really noob now. That's what you get when you didn't yet build real features with it :-1: Ha that sheds some new light on the discussion :)
I didn't know that either. So in case of proxies, the cloning of plain structures is required, to prevent their modifications in a non-observable manner (through original object references). Maybe that makes proxies a little bit less attractive? I mean they still seem cleaner from the implementation perspective, better separation of concerns and so on, but what benefits they really bring to the table for the user? An array can stay a type of an array, but it's still a different array (which we would currently like to get rid of by extending an Array, if it would be possible?)...
On the other hand, this limitation can streamline/simplify an autoconversion logic into something like this: "Anything, which can be automatically cloned is converted into observable - that is plain object, Array, Set, Map"
In that case, I think it makes sense to NOT clone objects, which are NOT made observable (autoconverted).
So in case of shallow([item1, item2])
the array itself is cloned, but the items are not (and the same rule is applied during reassigments). This makes the cloning logic also quite clear I think:
"Anything which is made observable is cloned"
That way, the cloning rules are aligned with autoconversion rules, quite easily understandable and predictable, don't you think?
EDIT: btw there is a reason to assume that extendable built-ins (Array/Set/Map) will be (or already are) supported the same way as proxies. (The ES6 allows extending these after all) So we could have a quite cosistent behavior in ES6 environment without cloning as well...
Agreed! I'll come with a fresh proposal later today :) we're getting somewhere
Op vr 18 nov. 2016 10:30 schreef urugator notifications@github.com:
I didn't know that either. So in case of proxies, the cloning of plain structures is required, to prevent modifications of objects in a non-observable manner (through original object references). Maybe that makes proxies a little bit less attractive? I mean they still seem cleaner from the implementation perspective, better sepeartion of concerns and so on, but what benefits they really bring to the table for the user? An array can stay a type of an array, but it's still a different array (which we would currently like to get rid of by extending an Array, if it would be possible?)...
On the other hand, this limitation can streamline/simplify an autoconversion logic into something like this: *"Anything, which can by automatically cloned is converted into observable
- that is plain object, Array, Set, Map"*
In that case, I think it makes sense to NOT clone objects, which are NOT made observable (autoconverted). So in case of shallow([item1, item2]) the array itself is cloned, but the items are not (and the same rule is applied during reassigments). This makes the cloning logic also quite clear I think: "Anything which is made observable is cloned"
That way, the cloning rules are aligned with autoconversion rules, quite easily understandable and predictable, don't you think?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/649#issuecomment-261487006, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhKDA7jd3lwFi27vGV8SFfWNwhzWnks5q_XA6gaJpZM4KwYbt .
but what benefits they really bring to the table for the user
- Proxied objects can behave just the same as originals (
Array.isArray(new Proxy([], {})) === true
,new Proxy([], {}) instanceof Array === true
)- Ability to track dynamic properties additions
- Ability to make anything observable (
new Proxy(new Date(), {})
)- Potential performance improvement
This is a proposal to kill the automatic conversion of objects / arrays to observables:
1. Stop automatic conversion of observables
While this, on one hand, is very cool and convenient for beginners, because any changes deep inside the todos collection can automatically be tracked. It also is a source of confusion. Although not often a cause of trouble, the mutation-of-the-righ-hand-side-of-an assignment (or
push
in the above example) is weird and hard to track if you didn't expect it. Several people already run into that. See for example #644So after this proposal one would need to do the following:
2. Kill modifiers
Currently, MobX provides an opt out mechanism of this behavior, modifiers like
asFlat
andasReference
.For example modifying the original listing to
todos = observable(asFlat([]))
would have prevented the behavior. So it can be opted-out. Since this proposal removes automagic observable conversion, the need to know about these modifiers is gone and they can be removed. (computed
,asMap
andasStructure
are still useful modifiers though). This solves a lot of design questions about the most convenient api around modifiers. See for example #211, #197, #563.
observable(object)
should clone instead of enhanceThe above example shows a nice inconsistency between observable objects and arrays (and maps). Objects being converted keep their identity, while arrays produce a fresh object (originally due to limitations in ES5). However to keep things explicit and predictable, I think it is nicer to prevent objects from being enhanced in place, and instead produce fresh objects. In other words, currently
observable(object)
is an alias forextendObservable(object, object)
. With this proposalobservable(object)
would be the same asextendObservable({}, object)
4. Migration path / questions
@observable
decorator is barely effected. Except when assigning objects, arrays. Should those be converted automatically to observables one level deep? E.g. should one need to write@observable todos = observable([])
or should@observable todos = []
suffice (and convert[]
) still automatically? Or should the whole api be more more explicit:@observable todos = observableArray()
?observable
on an already observable thing should probably warnTL; DR
I think this proposal reduces the perceived level of magic introduced by MobX, as the developer is now completely in control when objects become observable and has to be explicit about this. This might reduce the wow-factor for beginners a little bit, but reduce the confusion for intermediate (and beginner) MobX users.
This is definitely a breaking change, so run time guidance on all cases where behavior is changed would be very important I think (unless api method names are also changed, in that case old behavior could live side by side for a while)