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 firing too soon in set function #16

Closed
jpchip opened this issue Oct 17, 2012 · 3 comments
Closed

Change event firing too soon in set function #16

jpchip opened this issue Oct 17, 2012 · 3 comments

Comments

@jpchip
Copy link

jpchip commented Oct 17, 2012

This might be a problem just for me, but I thought I'd report it anyways.

In the wrapping of the set function, the Backbone.model.set() call comes before the attributes are iterated though and have a bind event triggered on each of them:

// Delegating to Backbone's model.set().
ret = oldSet.call(this, attrs, options);

// Iterate through the attributes that were just set.
.each(.keys(attrs || {}), .bind(function(attr) {
// Trigger a custom "bind" event for each attribute that has changed, unless {bind:false} option.
if (( !
.isEqual(now[attr], val) || (options.unset && _.has(now, attr))))
this.trigger('bind:' + attr, attrs[attr], options);
}, this));

If the view is listening to a change event on the model to render, and the render function calls this.stickit(), the bindings end up being applied twice. Moving the call to the Backbone.model.set() to after the bind events are triggered seems to fix this:

// Iterate through the attributes that were just set.
.each(.keys(attrs || {}), .bind(function(attr) {
// Trigger a custom "bind" event for each attribute that has changed, unless {bind:false} option.
if (( !
.isEqual(now[attr], val) || (options.unset && _.has(now, attr))))
this.trigger('bind:' + attr, attrs[attr], options);
}, this));

// Delegating to Backbone's model.set().
ret = oldSet.call(this, attrs, options);

@delambo
Copy link
Member

delambo commented Oct 22, 2012

@jpchip I think I got a fix for your issue on master. As you stated, the model was being bound twice. To fix this, before the given model is re-bound, I unstick any bindings.

I have never re-rendered after binding, but I can see why it may be needed, and stickit shouldn't bug-out when it happens.

I created the following fiddles which render twice, look at the internal _modelBindings array, and display the number of bindings (should be 2):

http://jsfiddle.net/px6UP/3/ (fix in master; should show 2 model bindings)
http://jsfiddle.net/px6UP/4/ (bug in commit before master; should show 4 bindings)

Let me know if this fixes your issue, or if I just squashed another issue, so I can cut a 0.5.3 tag ASAP.

@jpchip
Copy link
Author

jpchip commented Oct 22, 2012

I tested your change, and it did not fix my issue. I still needed to put the "ret = oldSet.call(this, attrs, options);" call after the bind triggering. I might just be doing something wrong in my view. I will see if I can set up a fiddle showing the error occurring. I guess you squashed another bug though, so that's good! Thanks for looking into the issue!

@delambo
Copy link
Member

delambo commented Oct 23, 2012

@jpchip I setup the following fiddle which re-renders after every change in the model:

http://jsfiddle.net/px6UP/5/

My fix on master definitely fixes the model being re-bound every time render fires.

I'm going to close this issue for now. Feel free to reopen if you can replicate or better describe your issue. Also, don't hesitate to ask questions.

@delambo delambo closed this as completed Oct 23, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants