Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change event options parameter not detailed enough #38

Open
virajsanghvi opened this issue Jun 20, 2012 · 5 comments
Open

change event options parameter not detailed enough #38

virajsanghvi opened this issue Jun 20, 2012 · 5 comments
Labels

Comments

@virajsanghvi
Copy link

The change event handler is passed the model and an options object that contains the attributes that have changed. Because you're calling the super's set(), its just specifying the top level attribute that changed. Ideally, this would specify the actual attributes that changed, similar to Backbone.DeepModel. Basically, there's a loss of information with what actually changed, and binding to the all event to get the change:* events has the problem that its not possible to dedupe changes.

Using model.changedAttributes() doesn't handle this use case because it accumulates all changes to the model

The use case I'm trying to support to syncing a model to a plain old javascript object for use with JsViews, and I would like to do this as efficiently as possible.

I'm guessing this isn't something you'd be able to support, but it would be nice.

@afeld
Copy link
Owner

afeld commented Jun 20, 2012

I think this may be a duplicate of #35... correct me if I'm wrong.

@afeld afeld closed this as completed Jun 20, 2012
@virajsanghvi
Copy link
Author

I'm pretty sure this is a separate issue. From what I understand:

(1) changedAttributes() will tell you all the attributes that have been changed since the last sync

(2) in the change event handler, function (model, options) {}, options.changes tells you the attributes that have changed in this change

This is at least the functionality I've seen in Backbone (it may change to support the features you're providing in NestedModel). #35 handles (1), but (2) is still an issue. options.changes only contains the top level attribute.

A basic test:

var m = new Backbone.NestedModel({foo: { bar: 0 });
var changes;
m.on('change', function (model, options) {
changes = options.changes;
});
m.set('foo.bar', 1);
deepEqual(changes, { foo: { bar: true } }); // or deepEqual(changes, { 'foo.bar': true });
// right now, changes == { foo : true }

@virajsanghvi
Copy link
Author

Sorry for the bump, @afeld, but just wondering what your take on this issue was, and if there was any chance of there being an option to provide this type type of information in the callback.

Thanks!

@afeld
Copy link
Owner

afeld commented Jun 26, 2012

Hey, sorry for not responding sooner. I actually wasn't aware of options.changes - it's not in the general documentation. Wouldn't be a difficult addition, but I probably can't get to it til this weekend. Pull requests are welcome, though :-)

@afeld afeld reopened this Jun 26, 2012
@gkatsev
Copy link
Collaborator

gkatsev commented Jan 11, 2014

Can you please retest using the backbone 1.x supported backbone-nested that's currently in master? It's possible that with how things are working now, we're getting this now. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants