jashkenas / backbone

Give your JS App some Backbone with Models, Views, Collections, and Events
http://backbonejs.org
MIT License
28.09k stars 5.38k forks source link

model.changed and model.changedAttributes do not work as advertised. #2374

Closed mikesnare closed 11 years ago

mikesnare commented 11 years ago

I see there's been a lot of back-and-forth over some recent changes to how model changes are tracked and published. I recently upgraded and was bitten in several places in our code where we were using set() with the silent flag to build up a change set and then calling change() once to fire a single event to update views. In my opinion, that was a very nice feature that is now gone and it would be awesome if it came back (though I realize you guys have some opinions as to why you don't want people to use silent sets, and I'm not going to argue with you on that). However, the docs clearly state the following:

http://backbonejs.org/#Model-set

If any of the attributes change the model's state, a "change" event will be triggered, unless {silent: true} is passed as an option.

http://backbonejs.org/#Model-changed

The changed property is the internal hash containing all the attributes that have changed since the last "change" event was triggered. Please do not update changed directly since its state is internally maintained by set. A copy of changed can be acquired from changedAttributes.

Per the docs, the 'changed' attributes should not be reset until a change event is triggered. I would expect the following, then:

var model = new Backbone.Model();
model.set('foo', 'bar', {silent : true});
model.set('bar', 'baz', {silent : true});
model.set('baz', 'bing', {silent : true});

/**
 * The result of this call should be
 * {
 *    foo: 'bar',
 *    bar: 'baz',
 *    baz: 'bing'
 * }
 */
model.changedAttributes();

Instead, the result is just {baz : 'bing'}. Rather than simply update the documentation to match the behavior, how difficult would it be to make the behavior match the documentation?

braddunbar commented 11 years ago

Hi @mikesnare, thanks for pointing this out! You're correct that the docs are inconsistent when applied to silent sets. This is why I would recommend you avoid silent sets and use custom options instead. Also, silent sets have been removed in master which makes the documentation there consistent.

tgriesser commented 11 years ago

And also, see #2113 for more information about the removal of the silent set.

braddunbar commented 11 years ago

Yes, definitely. Thanks Tim!

mikesnare commented 11 years ago

Ugh. The whole concept of silent:true is gone? That's disappointing.

Imagine you have an index view on a collection. Imagine the user does something in the UI that is a bulk operation on the models in the collection. For performance reasons, it seems perfectly reasonable and desireable to be able to iterate the collection and update each model with a new attribute with the silent option. Even with the short-circuiting described in #2113 the event gets triggered for every single model, for every single listener. That's a potentially significant performance drawback, as well as a development problem for every developer who now needs to update every single event handler to check for a silent flag?

I have code that does exactly what I described, and after doing the iteration I fire a custom event that says "attribute X on all models in the collection has been updated" and it's just up to me to know to to listen for that anywhere I need to listen for changes to that attribute.

I appreciate all the work you guys do -- I really like the Backbone code -- but I don't think I like this change. Why is {silent: true} considered such an anti-pattern? Does it just complicate the code too much (which I could understand). Maybe I'm just dreading my next upgrade...

Thanks for the response, btw.

braddunbar commented 11 years ago

As for performance drawbacks, the shenanigans going on in Model#set before this change were far more costly than triggering events so it's a bit of a moot point. I imagine you're listening on the collection for change events and alerting the user but if I've got that wrong please correct me. You should be able to change something like this

collection.on('change', function(){
  alert('...');
});

to something like this

collection.on('change', function(model, options){
  if (!options.silent) alert('...');
});

I'm sorry that this causes you a headache, but I think that these changes will cause you less of them in the long run. :)

mikesnare commented 11 years ago

What I was actually doing was responding to a user's desire to "upgrade" all elements on the page by sending a request to the server to do this operation in bulk, then (on success) iterating all models in the collection and silently setting an attribute to reflect the change.

Upgrades can be done on individual models, so the view representing a single instance must have a change listener on that attribute. Unfortunately, the view that represents the individual model renders differently enough based on that single attribute change that having each one trigger for each model is not a realistic option from a performance standpoint since what I've observed is that if each view responds to each individual change, we have more performance issues than if I have the parent index view iterate each view (on that custom bulk event) and tell them to update all at once. As I type that out, it doesn't seem to make sense, but that is most certainly what I saw at that time, and that is why I came up with the bulkchange event at that time.

As long as the performance doesn't degrade when I have to put this silent check in the view's listeners and events are fired, it should be fine (if not a bit annoying to have to do the refactoring).

Again, thanks for the response/info.

braddunbar commented 11 years ago

Ok, let us know if you have trouble with the change so we can make it as painless as possible.