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

jQuery events are not unbound when stickit is called multiple times #66

Closed
jr314159 opened this Issue Feb 10, 2013 · 16 comments

Comments

Projects
None yet
3 participants
@jr314159
Contributor

jr314159 commented Feb 10, 2013

It seems that the switch from Backbone delegated events to jQuery events has introduced a bug whereby re-rendering a Backbone view and calling stickit again causes the bound elements to continue to set attributes on the model.

For example, I have a view that I render multiple times. Each time I render, I call stickit(). The first time it renders, there are no problems. But the second time, when I try to enter text into a field it keeps getting replaced with the previously set value from the model.

I see that stickit() calls unstickModel, but judging from the behavior it seems that the jQuery events are staying around. I see that you've wrapped view.remove to unbind these events but when I rerender I don't call view.remove.

I might be wrong and there is some other issue going on but we just upgraded stickit and it appears that this is what's causing the problem.

@jr314159

This comment has been minimized.

Show comment
Hide comment
@jr314159

jr314159 Feb 10, 2013

Contributor

And I can confirm that adding this.$el.off('.stickit' + this.cid) to the beginning of my render function fixes the problem.

Should this line be added to unstickModel?

Contributor

jr314159 commented Feb 10, 2013

And I can confirm that adding this.$el.off('.stickit' + this.cid) to the beginning of my render function fixes the problem.

Should this line be added to unstickModel?

@delambo

This comment has been minimized.

Show comment
Hide comment
@delambo

delambo Feb 10, 2013

Member

Ah, right, when stickit used view.events, events were always un-delegated before being re-delegated. Now that we are using $.off and $.on we should probably off events for a view before delegating. That way you wouldn't have to manually off events every time you (re-)render.

But you bring up a good point in that it would be beneficial to off events in unstickModel. With that change, we might want to rename unstickModel to unstickit.

Member

delambo commented Feb 10, 2013

Ah, right, when stickit used view.events, events were always un-delegated before being re-delegated. Now that we are using $.off and $.on we should probably off events for a view before delegating. That way you wouldn't have to manually off events every time you (re-)render.

But you bring up a good point in that it would be beneficial to off events in unstickModel. With that change, we might want to rename unstickModel to unstickit.

@jr314159

This comment has been minimized.

Show comment
Hide comment
@jr314159

jr314159 Feb 10, 2013

Contributor

Sounds good. Although unstickmodel is a public method and I don't know if
it makes sense to rename it. Should I make a pull request?
On Feb 10, 2013 5:23 PM, "Matthew DeLambo" notifications@github.com wrote:

Ah, right, when stickit used view.events, events were always un-delegated
before being re-delegated. Now that we are using $.off and $.on we should
probably off events for a view before delegating. That way you wouldn't
have to manually off events every time you (re-)render.

But you bring up a good point in that it would be beneficial to offevents in
unstickModel. With that change, we might want to rename unstickModel to
unstickit.


Reply to this email directly or view it on GitHubhttps://github.com//issues/66#issuecomment-13362686..

Contributor

jr314159 commented Feb 10, 2013

Sounds good. Although unstickmodel is a public method and I don't know if
it makes sense to rename it. Should I make a pull request?
On Feb 10, 2013 5:23 PM, "Matthew DeLambo" notifications@github.com wrote:

Ah, right, when stickit used view.events, events were always un-delegated
before being re-delegated. Now that we are using $.off and $.on we should
probably off events for a view before delegating. That way you wouldn't
have to manually off events every time you (re-)render.

But you bring up a good point in that it would be beneficial to offevents in
unstickModel. With that change, we might want to rename unstickModel to
unstickit.


Reply to this email directly or view it on GitHubhttps://github.com//issues/66#issuecomment-13362686..

@delambo

This comment has been minimized.

Show comment
Hide comment
@delambo

delambo Feb 10, 2013

Member

With the added functionality, I think it makes sense to change unstickModel to unstickit. PRs are always welcome!

Member

delambo commented Feb 10, 2013

With the added functionality, I think it makes sense to change unstickModel to unstickit. PRs are always welcome!

@jr314159

This comment has been minimized.

Show comment
Hide comment
@jr314159

jr314159 Feb 11, 2013

Contributor

#67

Contributor

jr314159 commented Feb 11, 2013

#67

@delambo

This comment has been minimized.

Show comment
Hide comment
@delambo

delambo Feb 11, 2013

Member

Ack, there is a little problem with this idea/implementation. Currently, the api allows for stickit() to be called multiple times in a view and old bindings are retained while new bindings are added. I don't use this feature much and I'm not sure if it is popular. It's probably not very common to re-render views often when using stickit, but that might be a more common, or better supported, use case.

We definitely should fix the re-render problem but should the api (multiple calls with retained bindings) be maintained?

Member

delambo commented Feb 11, 2013

Ack, there is a little problem with this idea/implementation. Currently, the api allows for stickit() to be called multiple times in a view and old bindings are retained while new bindings are added. I don't use this feature much and I'm not sure if it is popular. It's probably not very common to re-render views often when using stickit, but that might be a more common, or better supported, use case.

We definitely should fix the re-render problem but should the api (multiple calls with retained bindings) be maintained?

@jr314159

This comment has been minimized.

Show comment
Hide comment
@jr314159

jr314159 Feb 11, 2013

Contributor

I guess I don't understand. Previously, calling stickit multiple times
would call unstickModel each time. When DOM events were managed by Backbone
and not jQuery, it was possible to call stickit multiple times on a view
without creating buggy behavior where zombie event bindings would hijack
user input. What was the purpose of calling unstickModel() at the beginning
of stickit() if not to allow stickit to be called repeatedly without
creating duplicate and conflicting bindings?

Anyway, for us it's very common to re-render views when using stickit. We
just use stickit to keep our models and form elements in sync, not to
render data into our views or hide and show elements.

On Sun, Feb 10, 2013 at 11:04 PM, Matthew DeLambo
notifications@github.comwrote:

Ack, there is a little problem with this idea/implementation. Currently,
the api allows for stickit() to be called multiple times in a view and
old bindings are retained while new bindings are added. I don't use this
feature much and I'm not sure if it is popular. It's probably not very
common to re-render views often when using stickit, but that might be a
more common, or better supported, use case.

We definitely should fix the re-render problem but should the api
(multiple calls with retained bindings) be maintained?


Reply to this email directly or view it on GitHubhttps://github.com//issues/66#issuecomment-13367805..

Contributor

jr314159 commented Feb 11, 2013

I guess I don't understand. Previously, calling stickit multiple times
would call unstickModel each time. When DOM events were managed by Backbone
and not jQuery, it was possible to call stickit multiple times on a view
without creating buggy behavior where zombie event bindings would hijack
user input. What was the purpose of calling unstickModel() at the beginning
of stickit() if not to allow stickit to be called repeatedly without
creating duplicate and conflicting bindings?

Anyway, for us it's very common to re-render views when using stickit. We
just use stickit to keep our models and form elements in sync, not to
render data into our views or hide and show elements.

On Sun, Feb 10, 2013 at 11:04 PM, Matthew DeLambo
notifications@github.comwrote:

Ack, there is a little problem with this idea/implementation. Currently,
the api allows for stickit() to be called multiple times in a view and
old bindings are retained while new bindings are added. I don't use this
feature much and I'm not sure if it is popular. It's probably not very
common to re-render views often when using stickit, but that might be a
more common, or better supported, use case.

We definitely should fix the re-render problem but should the api
(multiple calls with retained bindings) be maintained?


Reply to this email directly or view it on GitHubhttps://github.com//issues/66#issuecomment-13367805..

@andriijas

This comment has been minimized.

Show comment
Hide comment
@andriijas

andriijas Feb 11, 2013

Contributor

By using stickit it find i re-render views less frequently as well however.

In 90% of the times where i "restick" its due to the fact that the collections that I source to selectOptions have changed and should refresh. Therefore it has always been convenient to not have to unstick before resticking.

I havent stumbled up on a use case where multiple binding hashes could be used, or adding/removing bindings dynamically. I just setup all my bindings in the bindings hash and since we are using event delegation, those where a dom element is present just works.

To only only try to have 1 model or 1 collection per view makes it easy to keep away from complicated views. I rather have sub views which communicates through mediator pattern.

Contributor

andriijas commented Feb 11, 2013

By using stickit it find i re-render views less frequently as well however.

In 90% of the times where i "restick" its due to the fact that the collections that I source to selectOptions have changed and should refresh. Therefore it has always been convenient to not have to unstick before resticking.

I havent stumbled up on a use case where multiple binding hashes could be used, or adding/removing bindings dynamically. I just setup all my bindings in the bindings hash and since we are using event delegation, those where a dom element is present just works.

To only only try to have 1 model or 1 collection per view makes it easy to keep away from complicated views. I rather have sub views which communicates through mediator pattern.

@delambo

This comment has been minimized.

Show comment
Hide comment
@delambo

delambo Feb 11, 2013

Member

@jr314159 - check out the unit tests.

I think that implementation and breaking the api will be ok. Give me some time to change and write-up some unit tests to verify your fix.

Member

delambo commented Feb 11, 2013

@jr314159 - check out the unit tests.

I think that implementation and breaking the api will be ok. Give me some time to change and write-up some unit tests to verify your fix.

@jr314159

This comment has been minimized.

Show comment
Hide comment
@jr314159

jr314159 Feb 11, 2013

Contributor

Oh, I think I see. So unstickit and unstickView would also need to accept an optional model argument. And instead of creating one jQuery event namespace for a view, we would need to create a new namespace for each view+model combination, like .stickit + view.cid + model.id. Is that the idea?

Contributor

jr314159 commented Feb 11, 2013

Oh, I think I see. So unstickit and unstickView would also need to accept an optional model argument. And instead of creating one jQuery event namespace for a view, we would need to create a new namespace for each view+model combination, like .stickit + view.cid + model.id. Is that the idea?

@delambo

This comment has been minimized.

Show comment
Hide comment
@delambo

delambo Feb 11, 2013

Member

Yeah, I think we can simplify it by merging it all into one function - unstickit - since it probably isn't going to be a common use case to only unstick the view bindings or the model bindings. We can also keep it simple by calling unstickit(model) at the top of stickit.

Member

delambo commented Feb 11, 2013

Yeah, I think we can simplify it by merging it all into one function - unstickit - since it probably isn't going to be a common use case to only unstick the view bindings or the model bindings. We can also keep it simple by calling unstickit(model) at the top of stickit.

@delambo

This comment has been minimized.

Show comment
Hide comment
@delambo

delambo Feb 13, 2013

Member

I reworked PR #67 a little bit. I removed unstickModel and I took your suggestion on using the model.cid as a namespace. Now, a model is paired with a bindings configuration, so if you unbind a model with unstickit, then you unbind the corresponding view events as well.

@jr314159 - thanks for catching this issue. Can you try it out on your code base?

Member

delambo commented Feb 13, 2013

I reworked PR #67 a little bit. I removed unstickModel and I took your suggestion on using the model.cid as a namespace. Now, a model is paired with a bindings configuration, so if you unbind a model with unstickit, then you unbind the corresponding view events as well.

@jr314159 - thanks for catching this issue. Can you try it out on your code base?

@delambo delambo closed this Feb 13, 2013

@jr314159

This comment has been minimized.

Show comment
Hide comment
@jr314159

jr314159 Feb 13, 2013

Contributor

It's working great, thanks!

Contributor

jr314159 commented Feb 13, 2013

It's working great, thanks!

@andriijas

This comment has been minimized.

Show comment
Hide comment
@andriijas

andriijas Feb 13, 2013

Contributor

I have not been following the work that led up until unstickit - is all I need to do once I update change all my unstickModel to unstickit?

Contributor

andriijas commented Feb 13, 2013

I have not been following the work that led up until unstickit - is all I need to do once I update change all my unstickModel to unstickit?

@delambo

This comment has been minimized.

Show comment
Hide comment
@delambo

delambo Feb 13, 2013

Member

Yeah, it should work the same, but unstickit will also unbind the $el events as well. We had to make this change since we stopped delegating to backbone's events object.

Also, one difference now is that a model and a bindings hash are namespaced together, so calling unstickit with a particular model should unstick the model and the corresponding $el events.

Member

delambo commented Feb 13, 2013

Yeah, it should work the same, but unstickit will also unbind the $el events as well. We had to make this change since we stopped delegating to backbone's events object.

Also, one difference now is that a model and a bindings hash are namespaced together, so calling unstickit with a particular model should unstick the model and the corresponding $el events.

@andriijas

This comment has been minimized.

Show comment
Hide comment
@andriijas

andriijas Feb 13, 2013

Contributor

Sounds reasonable!

Thanks!

Contributor

andriijas commented Feb 13, 2013

Sounds reasonable!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment