Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Make changed properties available in "change" event #1016

Closed
wants to merge 1 commit into from

4 participants

@Fauntleroy

When the change event fired I figured it would tell me what attributes had changed, but the information was strangely absent. I merely made this._changed available to the user via the change callback.

@jashkenas
Owner

@braddunbar -- what do you think about this?

Changed properties are already available in the "change" event, via .changedAttributes(), .hasChanged(), .previous() etc... We didn't pass them along in the past because it was extra computation to compute the diff -- but now the diff is available. This would be a breaking change, in the way proposed.

@Fauntleroy

Ah, .changedAttributes() works for me, though it is a teensy bit cumbersome. I had been leading myself in the wrong direction because of how changed attributes are returned in the "change:attribute" event.

@vincentbriglia

braddunbar@63d8845 returns a changes attribute on the options for change events

@Fauntleroy

After taking a look at it, it seems that changedAttributes and previousAttributes only work properly while still inside a change event. Calling either method after the change event has fired will merely compare the current attributes against themselves. If this is the expected behavior, it should be reflected in the documentation (which doesn't explicitly state where they can be used).

@braddunbar
Collaborator

@vincentbriglia Hmm...that's an unintentional side effect I hadn't noticed (it only passes the changes from the current set call). Looks like that will need to be passed in a private variable or a separate argument to change. Thanks for pointing it out!

@jashkenas You're right, cloning the changed attributes costs much less than computing the diff. It would definitely be useful for a large number of 'change' listeners since changedAttributes could be skipped. You're also right about the breaking change. I think this should go in options if it's included.

I am concerned about a few things though. Exposing _changed through options invites modification (not to mention general concerns about exposing a private variable). This could be fixed by cloning it but then we risk stale data in successive callbacks. Barring a solution that addresses this I am -1.

@jashkenas
Owner

I think sharing the data is fine here... we expose model.attributes after all. Just don't modify it.

Hoping to push out another point release tomorrow, perhaps -- so if you want to address these two things, they'd be part of it.

@braddunbar
Collaborator

@jashkenas Good point. I hadn't thought about attributes in the same way. Worries assuaged. :)

Would it make sense to just expose _changed as changed? That way it could be documented as read-only like attributes.

@Fauntleroy

_changed seems to be deleted right after the change events are sent, so its never available outside the event.

@jashkenas
Owner

Great idea -- just exposing it sounds nice.

@braddunbar braddunbar closed this pull request from a commit
@braddunbar braddunbar Fixes #1016 - Expose `changed`.
* Expose `changed` as a public hash of changes.
* Initialize `changed`, `_pending`, and `_silent`
  in the constructor instead of checking in `set`.
* Ensure `changed` matches `attributes`.
afbca72
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 16, 2012
  1. @Fauntleroy
This page is out of date. Refresh to see the latest.
Showing with 2 additions and 2 deletions.
  1. +1 −1  backbone.js
  2. +1 −1  test/model.js
View
2  backbone.js
@@ -400,7 +400,7 @@
}
while (this._moreChanges) {
this._moreChanges = false;
- this.trigger('change', this, options);
+ this.trigger('change', this, this._changed, options);
}
this._previousAttributes = _.clone(this.attributes);
delete this._changed;
View
2  test/model.js
@@ -304,7 +304,7 @@ $(document).ready(function() {
test("Model: change with options", function() {
var value;
var model = new Backbone.Model({name: 'Rob'});
- model.on('change', function(model, options) {
+ model.on('change', function(model, changed, options) {
value = options.prefix + model.get('name');
});
model.set({name: 'Bob'}, {silent: true});
Something went wrong with that request. Please try again.