gmac / backbone.epoxy

Declarative data binding and computed models for Backbone
http://epoxyjs.org
MIT License
615 stars 89 forks source link

Model triggering "change" events when nothing actually changed #97

Closed markerikson closed 10 years ago

markerikson commented 10 years ago

After upgrading from Epoxy 1.0.2 to Epoxy 1.2, we began seeing a couple very odd behaviors in our application (a stack overflow in one case, a view repeatedly re-rendering on every collection update in another even though nothing had actually changed).

Commit 2d46296b changed the logic for when Epoxy.Model triggers events for computed attributes. It has the following chunk:

if (!this.hasChanged()) {
   this.trigger('change', this);
}

I believe the intent of this line is that a "change" event will be triggered if no data attributes were changed in the superclass BB.Model, but some computed attributes were. However, this will also fire in the case where Model.set() was called, but both computed and data attributes did not change (ie, all values were identical).

I tried the following change to the condition:

if (!this.hasChanged() && !_.isEmpty(computedEvents)) {
   this.trigger('change', this);
}

That appeared to fix the errant behavior I was seeing.

gmac commented 10 years ago

Hmmm. Yup. Ugg, sequencing those events is difficult. I'm going to have to dig back down into how the underlying Backbone event dispatcher works... it may be that Epoxy should just tell Backbone to update silently, and then handle all event triggers itself in the sequence it needs to run them.

gmac commented 10 years ago

Really good report. So, looking over the underlying Backbone model set operation... there's no way to override handling event triggers without basically duplicating the Backbone set operation, which would be a versioning nightmare. SO – I think we're going to have to officiate a quirk of computed attributes: their individual change-attribute events fire AFTER the model's full "change" event fires. I've made this firing order consistent within the codebase, and added your fix here (thank you!) for making sure that computed attributes trigger a "change" event. See f795840.

Alas, building around the Backbone setter is tricky.