AmpersandJS / ampersand-state

Core state management object.
MIT License
141 stars 75 forks source link

Derived properties don't listen to change events from collections #66

Open samhashemi opened 10 years ago

samhashemi commented 10 years ago

Simplified example:

var Model = require('ampersand-model');
var Collection = require('ampersand-collection');

var Room = Model.extend({
  props: {
    size: ['number', true, 10],
  },
});

var Rooms = Collection.extend({
  model: Room,
});

var House = Model.extend({
  collections: {
    rooms: Rooms,
  },

  derived: {
    totalArea: {
      deps: ['rooms'],
      fn: function() {
        return this.rooms.reduce(function(a, b) {
          return a + b.size;
        }, 0);
      }
    }
  },
});

var house = new House({
  rooms: [{size: 12}]
});

console.log(house.totalArea); // => 12
house.rooms.at(0).size = 15;
console.log(house.totalArea); // => 12

Seems like this should work?

lukekarrys commented 10 years ago

@samhashemi Looking through the code it seems there is no event bubbling handler for collections (only children). I think this is an oversight and I'll get a PR opened to fix it.

There is one other issue with the example though. The deps for totalArea need to be ['rooms.size'].

For now you can instead specify rooms as part of children and it should work (until the PR is merged/published). Here's the full code for that model which works in my tests:

var House = Model.extend({
  children: {
    rooms: Rooms,
  },
  derived: {
    totalArea: {
      deps: ['rooms.size'],
      fn: function() {
        return this.rooms.reduce(function(a, b) {
          return a + b.size;
        }, 0);
      }
    }
  },
});
samhashemi commented 10 years ago

Moving it to children did the trick.

There is one other issue with the example though. The deps for totalArea need to be ['rooms.size'].

Any reason listening to the whole child model / collection isn't supported? In my real code almost any property change would result in a change to the derived property -- it's easier to say listen to them all.

lukekarrys commented 10 years ago

Maybe @HenrikJoreteg or @latentflip could chime in on why that isn't supported since they are most familiar with this code.

I took a look through the example and added logging for all on everything and there would have to be more logic to bubble the generic change events up to the parent since currently change:rooms is never triggered on house.

screen shot 2014-09-08 at 4 59 03 pm

lukekarrys commented 10 years ago

So after digging through the code even more (so I don't have to call on @HenrikJoreteg or @latentflip next time I get stumped on state stuff :smile:) I think I found a good solution to allow listening to the whole child model/collection.

I added the commit to #67 and we can discuss it over there.

tnulc commented 9 years ago

This seems like a pretty massive bug which is presenting quite a lot of issues in a project we're using ampersand in. Is there any way this fix could be fast tracked?

latentflip commented 9 years ago

@kirisu can you outline a bit more specifically the issues you're running into (i.e. what you're trying to do?). This is part bug/part by intention, but it's behaviour we're mid changing. Some more thoughts on use cases would be useful. I'm in gitter right now too if you want to hop in there to chat.

tnulc commented 9 years ago

@latentflip

Here's a basic example of what I'm doing:

var Model = require('ampersand-model');
var Collection = require('ampersand-collection');
var View = require('ampersand-view');

var Photo = Model.extend({
  props: {
    id: 'string'
  }
});

var Photos = Collection.extend({
  model: Photo
}); 

var Car = Model.extend({
  props: {
    id: 'string'
  },
  session: {
    photoCount: ['number', false, 0]
  },
  collections: {
    gallery: Photos
  },
  initialize: function() {
    this.setPhotoCount();
    this.gallery.on('add remove change', function() {
      this.setPhotoCount();
    }
  },
  setPhotoCount: function() {
    this.set('photoCount', this.gallery.length);
  },
});

var Page = View.extend({
  bindings: {
    'model.photoCount': '[data-hook~=photo-count]',
  }
});

Every time I add a photo to the collection, I need to explicitly set the photoCount value so that the bindings in my template update. What I'd rather be able to do is:

var Photo = Model.extend({
  props: {
    id: 'string'
  }
});

var Photos = Collection.extend({
  model: Photo
}); 

var Car = Model.extend({
  props: {
    id: 'string'
  },
  collections: {
    gallery: Photos
  },
});

var Page = View.extend({
  bindings: {
    'model.gallery.length': '[data-hook~=photo-count]'
  }
});

But in this instance, my bindings don't update so my views end up out of sync with my model.

jdiaz5513 commented 9 years ago

It's a tricky problem to solve. I use a mixin to deal with this for now...

https://gist.github.com/jdiaz5513/6716e23c4995711db020

Could publish this mixin if it proves to be useful to anyone else.

tnguyen14 commented 9 years ago

Has there been any progress/ fix for this issue?