{"action":"created","issue":{"url":"https://api.github.com/repos/mobxjs/mobx/issues/649","repository_url":"https://api.github.com/repos/mobxjs/mobx","labels_url":"https://api.github.com/repos/mobxjs/mobx/issues/649/labels{/name}","comments_url":"https://api.github.com/repos/mobxjs/mobx/issues/649/comments","events_url":"https://api.github.com/repos/mobxjs/mobx/issues/649/events","html_url":"https://github.com/mobxjs/mobx/issues/649","id":188904077,"number":649,"title":"Proposal: Kill automatic conversion to observables","user":{"login":"mweststrate","id":1820292,"avatar_url":"https://avatars.githubusercontent.com/u/1820292?v=3","gravatar_id":"","url":"https://api.github.com/users/mweststrate","html_url":"https://github.com/mweststrate","followers_url":"https://api.github.com/users/mweststrate/followers","following_url":"https://api.github.com/users/mweststrate/following{/other_user}","gists_url":"https://api.github.com/users/mweststrate/gists{/gist_id}","starred_url":"https://api.github.com/users/mweststrate/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mweststrate/subscriptions","organizations_url":"https://api.github.com/users/mweststrate/orgs","repos_url":"https://api.github.com/users/mweststrate/repos","events_url":"https://api.github.com/users/mweststrate/events{/privacy}","received_events_url":"https://api.github.com/users/mweststrate/received_events","type":"User","site_admin":false},"labels":[{"id":289112564,"url":"https://api.github.com/repos/mobxjs/mobx/labels/discuss","name":"discuss","color":"006b75","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":18,"created_at":"2016-11-12T10:15:08Z","updated_at":"2016-11-12T21:33:50Z","closed_at":null,"body":"This is a proposal to kill the automatic conversion of objects / arrays to observables:\r\n\r\n```javascript\r\nconst todo = { done: false}\r\nisObservable(todo) // false\r\n\r\nconst todos = observable([]) // observable array\r\ntodos.push(todo)\r\nisObservable(todo) // true\r\n```\r\n\r\n## 1. Stop automatic conversion of observables\r\n\r\nWhile 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 #644\r\n\r\nSo after this proposal one would need to do the following:\r\n\r\n```javascript\r\nconst todo = observable({ done: false})\r\nisObservable(todo) // true\r\n\r\nconst todos = observable([])\r\ntodos.push(todo)\r\nisObservable(todo) // true\r\n```\r\n\r\n## 2. Kill modifiers\r\n\r\nCurrently, MobX provides an opt out mechanism of this behavior, modifiers like `asFlat` and `asReference`. \r\n\r\nFor example modifying the original listing to `todos = observable(asFlat([]))` would have prevented the behavior. So it can be opted-out.\r\nSince this proposal removes automagic observable conversion, the need to know about these modifiers is gone and they can be removed. (`computed`, `asMap` and `asStructure` are still useful modifiers though). This solves a lot of design questions about the most convenient api around modifiers. See for example #211, #197, #56 \r\n\r\n## 3. `observable(object)` should clone instead of enhance\r\n\r\n```javascript\r\nconst todo1 = { done: false }\r\nconst todo2 = observable(todo1)\r\ntodo1 === todo2 // true\r\n\r\nconst todos1 = []\r\nconst todos2 = observable(todos1)\r\ntodos1 === todos2 // false\r\n```\r\n\r\nThe 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 for `extendObservable(object, object)`. With this proposal `observable(object)` would be the same as `extendObservable({}, object)`\r\n\r\n## 4. Migration path / questions\r\n\r\n1. 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\r\n2. `@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()` ?\r\n3. calling `observable` on an already observable thing should probably warn\r\n\r\n## TL; DR\r\n\r\nI 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. \r\n\r\nThis 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)"},"comment":{"url":"https://api.github.com/repos/mobxjs/mobx/issues/comments/260149813","html_url":"https://github.com/mobxjs/mobx/issues/649#issuecomment-260149813","issue_url":"https://api.github.com/repos/mobxjs/mobx/issues/649","id":260149813,"user":{"login":"mweststrate","id":1820292,"avatar_url":"https://avatars.githubusercontent.com/u/1820292?v=3","gravatar_id":"","url":"https://api.github.com/users/mweststrate","html_url":"https://github.com/mweststrate","followers_url":"https://api.github.com/users/mweststrate/followers","following_url":"https://api.github.com/users/mweststrate/following{/other_user}","gists_url":"https://api.github.com/users/mweststrate/gists{/gist_id}","starred_url":"https://api.github.com/users/mweststrate/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mweststrate/subscriptions","organizations_url":"https://api.github.com/users/mweststrate/orgs","repos_url":"https://api.github.com/users/mweststrate/repos","events_url":"https://api.github.com/users/mweststrate/events{/privacy}","received_events_url":"https://api.github.com/users/mweststrate/received_events","type":"User","site_admin":false},"created_at":"2016-11-12T21:33:50Z","updated_at":"2016-11-12T21:33:50Z","body":"@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:\r\n```\r\nconst store = deepObservable({ todos: [ { completed: true } ]})\r\nisObservable(store.todos[0], \"completed\") // true\r\n\r\nstore.todos.push({completed: false})\r\nisObservable(store.todos[1], \"completed\")  // ... ?\r\n// what is expected now?\r\n// 1. true, automagic conversion of original\r\n// 2. true, automagic conversion in clone\r\n// 3. false. Confusing, first object observable, second not?\r\n// 4. throw / warn somehow you probably wanted to add an observable\r\n```\r\n\r\n\r\n\r\n"}}
{"actor":{"display_login":"mweststrate"}}