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

extendObservable not creating observable for all properties #53

Closed
sunny-g opened this issue Nov 14, 2015 · 9 comments
Closed

extendObservable not creating observable for all properties #53

sunny-g opened this issue Nov 14, 2015 · 9 comments

Comments

@sunny-g
Copy link

sunny-g commented Nov 14, 2015

I have this code here, taken almost straight from the docs:

function Todo(task, store) {
  this.store = store;
  mobservable.extendObservable(this, {
    task: task,
    completed: false,
    count: 0
  })
  console.log(mobservable.isObservable(this.count));   // is false
}

I thought all key-value pairs are made into observables as per: the docs?

@mweststrate
Copy link
Member

Hi @sunny-g,

in your example you pass the value of count to isObservable, which is just a boolean, which isn't false. For properties, pass the attribute name as string; isObservable(this, "count") should return true.

@sunny-g
Copy link
Author

sunny-g commented Nov 14, 2015

@mweststrate oh duh, thanks for that clarification!

With that in mind, how would I set up an observer function specifically for that property? this.count.observe obviously won't work, and mobservable.observe(this... is throwing an error (probably because the instance itself is not an observable).

The end goal is to tell the TodoStore to re-sort it's todos array based on the count anytime a todo's count has changed. I was doing it in my React render function, but thought that that was wasteful. Am I going about this functionality the wrong way?

@mweststrate
Copy link
Member

You could do something along these lines:

let prevCount = x;
autorun(() => {
    if (this.todos.length != prevCount) {
          prevCount = this.todos.length;
          // alter todo's
    }
}

Besides that I can extend observe to accept a second property parameter as well, so that it is possible to observe individual properties. That might be convenient in cases like these where you want access to the previous value

@sunny-g
Copy link
Author

sunny-g commented Nov 16, 2015

Ahh I think you may have slightly misunderstood my use case, though I like the approach you presented for other situations.

Say each of my todos has a click count and I want the ones with the most clicks to be at the top of the list. If I was displaying the count next to the todo, mobservable would handle this perfectly (as it does in your getting started example).

However, I need to do some kind of sorting operation on some event (on each click or something else) to keep the list in order. Conventionally, I would emit an event to the store or call a function in the store on every click, telling it to sort itself.

One possible approach would be to do this within the todo item's constructor (with the currently-non-existant second param property as you suggested):

this.observe('count', () => {
  this.store.sort();
})

but I'm sure there are approaches as well.

Going off the last example in your getting-started, I made these changes and got the minimum amount re-renders as expected:

ObservableTodoStore.prototype.sort = function() {
  this.todos.sort(function(a, b) {
    return a.count - b.count;
  });
}

var todoStore = new ObservableTodoStore();

// and in the onToggleCompleted function:
onToggleCompleted: function() {
  var todo = this.props.todo;
  todo.completed = !todo.completed;
  todo.count++
  todoStore.sort();
},

I guess I'm mainly just curious as to what the best practices are for these kinds of data relationships and common patterns when using observables as opposed to events listeners or something else entirely. With listeners, you just emit the event and the action happens; with observables, if you have cascading changes, it becomes too easy to trigger too many observe and autorun functions.

@sunny-g sunny-g closed this as completed Nov 16, 2015
@sunny-g sunny-g reopened this Nov 16, 2015
@mweststrate
Copy link
Member

Ah thanks for the extensive explanation of your use case. To me it seems that your sorting doesn't need to be a 'side effect' but can be derived purely from your data. So personally I would approach it roughly as follows:

/// in constructor
mobservable.extendObservable(this, {
  sortedTodos: function() {
     var sortedTodos = this.todos.slice(); 
     sortedTodos.sort( /* the sort function */);
     return sortedTodos;
  }
});

This way you just keep the sorting a pure derivation of your state, and mobservable will figure out itself when the sorted list needs to be updated. Just make sure you perform all mutations on your original list and not on the sorted one ;-).

Note that sorted is performed one a clone, if you use lodash you could just write it as return lodash.sort(this.todos.slice(), sortFunction).

I hope this helps!

@sunny-g
Copy link
Author

sunny-g commented Nov 16, 2015

Thanks for the quick update! So then that would mean that I should pass todoStore.sortedTodos as props to the TodoList component, allowing us to store the todos as is and then just pass a cloned version of it to our components, correct?

@mweststrate
Copy link
Member

Jup. Often I just pass complete stores around, so that their methods are
available in the components for modification.

On Mon, Nov 16, 2015 at 8:37 PM, Sunny Gonnabathula <
notifications@github.com> wrote:

Thanks for the quick update! So then that would mean that I should pass
todoStore.sortedTodos as props to the TodoList component, allowing us to
store the todos as is and then just pass a cloned version of it to our
components, correct?


Reply to this email directly or view it on GitHub
#53 (comment)
.

@sunny-g
Copy link
Author

sunny-g commented Nov 16, 2015

Right on! Thanks so much for your help @mweststrate. This lib is pretty cool and the API you've exposed as well as the examples for using it so easily with React/React Native make it really powerful, though (for me at least) the simplicity is accompanied with an intellectual transition cost.

I'm going through the docs right now and will make a PR soon with some changes, but I'd also really like to blog about using mobservable with React with more intermediate to advanced architectural patterns, so expect to hear more questions from me soon if that's alright with you 😄

@sunny-g sunny-g closed this as completed Nov 16, 2015
@mweststrate
Copy link
Member

Thanks, PR's and blogs are really appreciated, so feel free to ask any further questions :)

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

No branches or pull requests

2 participants