psr7-demo icon indicating copy to clipboard operation
psr7-demo copied to clipboard

Calling model.save() fires observer twice

Open rickharrison opened this issue 10 years ago • 2 comments

If I have a model like so:

var ItemModel = FP.Model.extend({
  title: FP.attr('string'),
  completed: FP.attr('boolean', { default: false }),

  completedDidChange: function () {
    console.log('in here');
    this.save();
  }.observes('completed')
});

When completed is set to a different value, that observer is fired twice. If I remove the this.save() call, the observer is only fired once. Is this expected behavior or am I doing something wrong?

rickharrison avatar May 10 '14 03:05 rickharrison

What's probably happening is that the observer is being called by the initial set, then when it's saved Firebase triggers a child_changed on this value, which then notifies us the change here and then that calls the observer again.

This sounds like it would cause recursion because this then calls save, which then causes a callback, which then calls save, ... but because nothing has changed on the 2nd run round Firebase doesn't do anything and the cycle stops.

I guard against this in some places by checking whether the observer has been fired by the app or by Firebase:

completedDidChange: function () {
  if (this.get("changeCameFromFirebase")) { return; }
  this.save();
}.observes('completed')

It feels like a bit of a hack and it would be good for this to be handled automatically, any ideas on the best way of doing that would be great!

rlivsey avatar May 10 '14 15:05 rlivsey

Cool thanks, that is a good workaround for now.

So in setAttributeFromSnapshot it appears that the property differs from the current snapshot value of it. I.E this.get('key') differs from this.get('snapshot').val()[key]. Should it also check to see if the current value is set to the snapshot value before notifying of a change. Something like this perhaps:

if (currentData.hasOwnProperty(key) && currentData[key] === newVal) {
  return;
}

current.set(key, newVal);

if (Ember.isEqual(this.get(key), newVal)) {
  return;
}

this.settingFromFirebase(function(){
  this.notifyPropertyChange(attribute);
});

With the new part being:

if (Ember.isEqual(this.get(key), newVal)) {
  return;
}

Now, I am still new to firebase and I'm not that familiar with your code base so I could be totally off base with this suggestion. Let me know what you think!

rickharrison avatar May 11 '14 04:05 rickharrison