Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assigning plain object to observable property forces plain object to become observable #644

Closed
AlgoTrader opened this issue Nov 5, 2016 · 5 comments

Comments

@AlgoTrader
Copy link

The code that represents the problem:

class Test {
  @observable city = null;

  @action setCity(myCity) {
    this.city = myCity;
  }
}

let test = new Test();
let myCity = {id: 1, name: 'London'};
test.setCity(myCity);
console.log('myCity=', myCity);
  1. myCity is plain object.
  2. test.setCity(myCity) just assign this.city = myCity;
  3. Bingo! myCity is no longer plain object.

I have to clone right side of assignments to prevent 'readonly' objects to be converted into observable.

I certainly want my plain objects to stay plain when they are in the right side of assignment! Assignment should modify left side, not the right one.

@mweststrate
Copy link
Member

this is the intended default, but you can use modifiers to change this
behavior, see asReference

Op za 5 nov. 2016 11:29 schreef AlgoTrader [email protected]:

The code that represents the problem:

class Test {
@observable city = null;

@action setCity(myCity) {
this.city = myCity;
}
}

let test = new Test();
let myCity = {id: 1, name: 'London'};
test.setCity(myCity);
console.log('myCity=', myCity);

  1. myCity is plain object.
  2. test.setCity(myCity) just assign this.city = myCity;
  3. Bingo! myCity is no longer plain object.

I have to clone right side of assignments to prevent 'readonly' objects to
be converted into observable.

I certainly want my plain objects to stay plain when they are in the right
side of assignment! Assignment should modify left side, not the right one.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#644, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABvGhE5EH-mxul8aDJCFf3CXSHurjLkdks5q7FqDgaJpZM4KqOZV
.

@AlgoTrader
Copy link
Author

for observable(myStuff) it looks reasonable. for this.stuff = myStuff the modification of the right side breaks the JavaScript language semantics. I can never trust assignment, if assignment modify both left and right sides of assignment.

I spend a couple of hours debugging why my original data was broken. It is first time that I see the setter to modify both the property and value to be assigned.

@mweststrate
Copy link
Member

I agree this can be confusing. The reasoning though is as follow; if your state is minimally defined, state should only be part of the observables and not be lingering around elsewhere, and hence the automatic conversion is very practical / boilerplate free. It makes sure complex data structures like :

todos = observable([])
todos.push({ completed: false, title: "test"}) 

can be observed by default in their entirety.

Two alternatives are:

  1. need to make everything observable explicitly (e.g. todos.push(observable({completed: false, title: "test")). This becomes cumbersome quite quickly in most cases.
  2. clone everything value on assignment before making it observabe. But that will be confusing as well, just in a different way? And a little expensive? Not that this actually happens (currently) to arrays as well, so in that sense it would be consistent

That being said the current approach isn't elegant either. When MobX switches to proxies, things will be slightly better, but still have assignments with side effects on the right-hand side. If somebody has a solution that is both sound and intuitive I am always glad to hear suggestions :)

@AlgoTrader
Copy link
Author

I agree with both your and my reasons. I would suggest to somehow warn of operations that have side effects. I got my pain of 'immutable' data to be muted.

@mweststrate
Copy link
Member

Maybe actually will fix @AlgoTrader :) Feel free to chime in on the related / follow-up discussion in #649

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants