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

Only attempt unbind when __bindEvents is defined #1780

Merged
merged 1 commit into from Sep 8, 2015

Conversation

Projects
None yet
3 participants
@akagomez
Contributor

akagomez commented Jul 13, 2015

An error is thrown if unbind is called on a compute that has no bindings.

This PR addresses the issue by returning early when __bindEvents is falsy, as is done in event.js here: https://github.com/bitovi/canjs/blob/master/event/event.js#L196-L198

Closes #1779

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 13, 2015

Contributor

This error can and has served as a good warning that something is wrong. Are we sure we want to remove it?

Sent from my iPhone

On Jul 12, 2015, at 11:15 PM, Chris Gomez notifications@github.com wrote:

An error is thrown if unbind is called on a compute that has no bindings.

This PR addresses the issue by returning early when __bindEvents is falsy, as is done in event.js here: https://github.com/bitovi/canjs/blob/master/event/event.js#L196-L198

Closes #1779

You can view, comment on, or merge this pull request online at:

#1780

Commit Summary

Only attempt unbind when __bindEvents is defined
File Changes

M util/bind/bind.js (3)
Patch Links:

https://github.com/bitovi/canjs/pull/1780.patch
https://github.com/bitovi/canjs/pull/1780.diff

Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jul 13, 2015

This error can and has served as a good warning that something is wrong. Are we sure we want to remove it?

Sent from my iPhone

On Jul 12, 2015, at 11:15 PM, Chris Gomez notifications@github.com wrote:

An error is thrown if unbind is called on a compute that has no bindings.

This PR addresses the issue by returning early when __bindEvents is falsy, as is done in event.js here: https://github.com/bitovi/canjs/blob/master/event/event.js#L196-L198

Closes #1779

You can view, comment on, or merge this pull request online at:

#1780

Commit Summary

Only attempt unbind when __bindEvents is defined
File Changes

M util/bind/bind.js (3)
Patch Links:

https://github.com/bitovi/canjs/pull/1780.patch
https://github.com/bitovi/canjs/pull/1780.diff

Reply to this email directly or view it on GitHub.

@akagomez

This comment has been minimized.

Show comment
Hide comment
@akagomez

akagomez Jul 13, 2015

Contributor

I can't imagine we'd want to throw that error for the case I outlined.

What others exist? Can you think of some off hand?

Contributor

akagomez commented Jul 13, 2015

I can't imagine we'd want to throw that error for the case I outlined.

What others exist? Can you think of some off hand?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 13, 2015

Contributor

I'd want an error thrown for that case.

Sent from my iPhone

On Jul 13, 2015, at 6:24 AM, Chris Gomez notifications@github.com wrote:

I can't imagine we'd want to throw that error for the case I outlined.

What others exist? Can you think of some off hand?


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jul 13, 2015

I'd want an error thrown for that case.

Sent from my iPhone

On Jul 13, 2015, at 6:24 AM, Chris Gomez notifications@github.com wrote:

I can't imagine we'd want to throw that error for the case I outlined.

What others exist? Can you think of some off hand?


Reply to this email directly or view it on GitHub.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 13, 2015

Contributor

Calling unbind without arguments is invalid. Calling unbind that doesn't actually unbind is invalid.

I've never been a fan of how jQuery swallows errors. I don't think we should do it too.

Sent from my iPhone

On Jul 13, 2015, at 6:24 AM, Chris Gomez notifications@github.com wrote:

I can't imagine we'd want to throw that error for the case I outlined.

What others exist? Can you think of some off hand?


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jul 13, 2015

Calling unbind without arguments is invalid. Calling unbind that doesn't actually unbind is invalid.

I've never been a fan of how jQuery swallows errors. I don't think we should do it too.

Sent from my iPhone

On Jul 13, 2015, at 6:24 AM, Chris Gomez notifications@github.com wrote:

I can't imagine we'd want to throw that error for the case I outlined.

What others exist? Can you think of some off hand?


Reply to this email directly or view it on GitHub.

@akagomez

This comment has been minimized.

Show comment
Hide comment
@akagomez

akagomez Jul 13, 2015

Contributor

Maybe there's a better alternative to my use case then. How I can I safely remove all bindings from a compute, regardless of whether it is or isn't bound to already?

Contributor

akagomez commented Jul 13, 2015

Maybe there's a better alternative to my use case then. How I can I safely remove all bindings from a compute, regardless of whether it is or isn't bound to already?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 13, 2015

Contributor

Maybe is misunderstood what you are trying to do. Removing the error is different then making it possible to not pass an event handler and have all handlers removed.

Sent from my iPhone

On Jul 13, 2015, at 7:42 AM, Chris Gomez notifications@github.com wrote:

Maybe there's a better alternative to my use case then. How I can I remove all bindings from a compute?


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jul 13, 2015

Maybe is misunderstood what you are trying to do. Removing the error is different then making it possible to not pass an event handler and have all handlers removed.

Sent from my iPhone

On Jul 13, 2015, at 7:42 AM, Chris Gomez notifications@github.com wrote:

Maybe there's a better alternative to my use case then. How I can I remove all bindings from a compute?


Reply to this email directly or view it on GitHub.

@akagomez

This comment has been minimized.

Show comment
Hide comment
@akagomez

akagomez Jul 13, 2015

Contributor

You're right. I assumed that calling .unbind() without any arguments would remove all of the bindings. After getting this error I then assumed that if there were bindings to be removed, they would be.

I could swear I've confirmed this before. But now I'm not sure. I probably got this mixed up with how can.Map can unbind all handlers bound to a property: http://jsbin.com/yomimiyuce/edit?js,console,output

Contributor

akagomez commented Jul 13, 2015

You're right. I assumed that calling .unbind() without any arguments would remove all of the bindings. After getting this error I then assumed that if there were bindings to be removed, they would be.

I could swear I've confirmed this before. But now I'm not sure. I probably got this mixed up with how can.Map can unbind all handlers bound to a property: http://jsbin.com/yomimiyuce/edit?js,console,output

@akagomez

This comment has been minimized.

Show comment
Hide comment
@akagomez

akagomez Jul 14, 2015

Contributor

One slight clarification. Calling .unbind() without any arguments does in fact not remove any bindings.

You can remove all of the bindings of a particular type by calling .unbind([type]).

That said, doing so will still throw an error if there are no bindings at the time .unbind() is called on the compute. This PR addresses that specific issue.

Also, test added.

Contributor

akagomez commented Jul 14, 2015

One slight clarification. Calling .unbind() without any arguments does in fact not remove any bindings.

You can remove all of the bindings of a particular type by calling .unbind([type]).

That said, doing so will still throw an error if there are no bindings at the time .unbind() is called on the compute. This PR addresses that specific issue.

Also, test added.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 28, 2015

Contributor

Looks good. @akagomez anyway you can resolve the merge conflicts, make sure travis passes, and merge this into master?

Contributor

justinbmeyer commented Jul 28, 2015

Looks good. @akagomez anyway you can resolve the merge conflicts, make sure travis passes, and merge this into master?

@justinbmeyer justinbmeyer added the bug label Jul 28, 2015

@justinbmeyer justinbmeyer added this to the 2.2.8 milestone Jul 28, 2015

@akagomez

This comment has been minimized.

Show comment
Hide comment
@akagomez

akagomez Jul 30, 2015

Contributor

Rebased. Tests passing. Good to merge.

Contributor

akagomez commented Jul 30, 2015

Rebased. Tests passing. Good to merge.

daffl added a commit that referenced this pull request Sep 8, 2015

Merge pull request #1780 from bitovi/unbind-non-bound-compute
Only attempt unbind when __bindEvents is defined

@daffl daffl merged commit 3fca932 into master Sep 8, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@daffl daffl deleted the unbind-non-bound-compute branch Sep 8, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 15, 2015

Contributor

@akagomez cc @daffl I think I misunderstood what this pull request does. @akagomez can you confirm that it does not help actually remove all event bindings? If this is the case, I still don't understand why this is useful. An error is useful.

Contributor

justinbmeyer commented Sep 15, 2015

@akagomez cc @daffl I think I misunderstood what this pull request does. @akagomez can you confirm that it does not help actually remove all event bindings? If this is the case, I still don't understand why this is useful. An error is useful.

@akagomez

This comment has been minimized.

Show comment
Hide comment
@akagomez

akagomez Sep 15, 2015

Contributor

The confusion may be due to the fact that I opened the issue for the case .unbind(). That was in error. Calling .unbind() without an event "type" does nothing. See here: https://github.com/bitovi/canjs/blob/master/event/event.js#L199

I attributed that error to calling .unbind() without a type. In reality it was present even if a "type" was defined:

var c = can.compute(32);
c.unbind('change'); //-> "TypeError: Cannot read property 'undefined' of undefined
                 at can.unbindAndTeardown [as unbind] 

This PR prevents an error from being thrown if .unbind("type") is called and there are no bindings, which is consistent with most "remove" api's that I'm familiar with (e.g. el.removeClass('class'), map.removeAttr('attr'), etc)

Making .unbind() remove all event handlers of all types was not the intention of this PR.

Contributor

akagomez commented Sep 15, 2015

The confusion may be due to the fact that I opened the issue for the case .unbind(). That was in error. Calling .unbind() without an event "type" does nothing. See here: https://github.com/bitovi/canjs/blob/master/event/event.js#L199

I attributed that error to calling .unbind() without a type. In reality it was present even if a "type" was defined:

var c = can.compute(32);
c.unbind('change'); //-> "TypeError: Cannot read property 'undefined' of undefined
                 at can.unbindAndTeardown [as unbind] 

This PR prevents an error from being thrown if .unbind("type") is called and there are no bindings, which is consistent with most "remove" api's that I'm familiar with (e.g. el.removeClass('class'), map.removeAttr('attr'), etc)

Making .unbind() remove all event handlers of all types was not the intention of this PR.

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