fixes map unbind bug #1015

Merged
merged 1 commit into from Jun 10, 2014

Conversation

Projects
None yet
3 participants
@DVSoftware
Contributor

DVSoftware commented May 26, 2014

Fixes a bug i encountered while unbinding events on a map. If there are no events bound on a map, it throws an error, see http://jsfiddle.net/4ZxcL/

@daffl daffl added this to the 2.1.2 milestone May 27, 2014

daffl added a commit that referenced this pull request Jun 10, 2014

@daffl daffl merged commit 1c64a37 into canjs:master Jun 10, 2014

1 check passed

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

daffl added a commit that referenced this pull request Jun 10, 2014

daffl added a commit that referenced this pull request Jun 10, 2014

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jun 10, 2014

Contributor

So, I thought about this while writing this code. I've found that blowing things up when unbinding on events that were never bound useful for discovering errors in code.

Thoughts on this?

Contributor

justinbmeyer commented Jun 10, 2014

So, I thought about this while writing this code. I've found that blowing things up when unbinding on events that were never bound useful for discovering errors in code.

Thoughts on this?

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jun 10, 2014

Contributor

Maybe as a dev warning? Most DOM libraries don't error either when unbinding something that didn't have any events.

Contributor

daffl commented Jun 10, 2014

Maybe as a dev warning? Most DOM libraries don't error either when unbinding something that didn't have any events.

@DVSoftware

This comment has been minimized.

Show comment
Hide comment
@DVSoftware

DVSoftware Jun 10, 2014

Contributor

We have a really large code base, and after upgrading CanJS to latest, we started facing this bug, and we currently are not in a position to refactor all of the code to make this upgrade. I agree with daffl, most of the libraries don't error, they just unbind if events are set, and it makes sense. For example, i want to unbind all change listeners regardless there are any.

Contributor

DVSoftware commented Jun 10, 2014

We have a really large code base, and after upgrading CanJS to latest, we started facing this bug, and we currently are not in a position to refactor all of the code to make this upgrade. I agree with daffl, most of the libraries don't error, they just unbind if events are set, and it makes sense. For example, i want to unbind all change listeners regardless there are any.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jun 11, 2014

Contributor

I believe dojo and yui do.

Sent from my iPhone

On Jun 10, 2014, at 4:58 PM, David Luecke notifications@github.com wrote:

Maybe as a dev warning? Most DOM libraries don't error either when unbinding something that didn't have any events.


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jun 11, 2014

I believe dojo and yui do.

Sent from my iPhone

On Jun 10, 2014, at 4:58 PM, David Luecke notifications@github.com wrote:

Maybe as a dev warning? Most DOM libraries don't error either when unbinding something that didn't have any events.


Reply to this email directly or view it on GitHub.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jun 11, 2014

Contributor

Just to make sure everyone knows this ... map.unbind('foo') does not unbind all 'foo' events.

Sent from my iPhone

On Jun 10, 2014, at 5:16 PM, DVSoftware notifications@github.com wrote:

We have a really large code base, and after upgrading CanJS to latest, we started facing this bug, and we currently are not in a position to refactor all of the code to make this upgrade. I agree with daffl, most of the libraries don't error, they just unbind if events are set, and it makes sense. For example, i want to unbind all change listeners regardless there are any.


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jun 11, 2014

Just to make sure everyone knows this ... map.unbind('foo') does not unbind all 'foo' events.

Sent from my iPhone

On Jun 10, 2014, at 5:16 PM, DVSoftware notifications@github.com wrote:

We have a really large code base, and after upgrading CanJS to latest, we started facing this bug, and we currently are not in a position to refactor all of the code to make this upgrade. I agree with daffl, most of the libraries don't error, they just unbind if events are set, and it makes sense. For example, i want to unbind all change listeners regardless there are any.


Reply to this email directly or view it on GitHub.

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