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

Prevent errors if destroy is called multiple times on a Control instance #352

Merged
merged 3 commits into from May 1, 2013

Conversation

Projects
None yet
3 participants
@ccummings
Contributor

ccummings commented Apr 23, 2013

Calling destroy() more than once on a control instance results in a JavaScript error in can.Control.prototype.off as seen in this fiddle.

This is a foot gun that is easily disarmed by checking if this.element === null before attempting to do anything in destroy.

This typically happens when an instances' element is destroyed via DOM manipulation followed by one or more calls to destroy().

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Apr 9, 2013

Contributor

I think jQueryMX did this, but I removed it because why would you be calling destroy multiple times?

Contributor

justinbmeyer commented Apr 9, 2013

I think jQueryMX did this, but I removed it because why would you be calling destroy multiple times?

@ccummings

This comment has been minimized.

Show comment
Hide comment
@ccummings

ccummings Apr 9, 2013

Contributor

The most typical use case is an element getting destroyed and then having someone else call destroy() programmatically. Happens quite often in controls that manage other controls. This has also cropped up in test harnesses, where a sandbox is cleaned up after a test is run, but the test teardown also calls destroy on the instances it created.

Contributor

ccummings commented Apr 9, 2013

The most typical use case is an element getting destroyed and then having someone else call destroy() programmatically. Happens quite often in controls that manage other controls. This has also cropped up in test harnesses, where a sandbox is cleaned up after a test is run, but the test teardown also calls destroy on the instances it created.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Apr 18, 2013

Contributor

I can see that this might happen. If it is as easy as adding if(!this.element) return; before https://github.com/bitovi/canjs/blob/master/control/control.js#L702 I don't see why it shouldn't be fixed.

Contributor

daffl commented Apr 18, 2013

I can see that this might happen. If it is as easy as adding if(!this.element) return; before https://github.com/bitovi/canjs/blob/master/control/control.js#L702 I don't see why it shouldn't be fixed.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Apr 18, 2013

Contributor

I wanted this error in the past because it meant I was doing something else wrong.

Curtis, can you share some code that should be fine that creates this error.

Sent from my iPhone

On Apr 18, 2013, at 9:53 AM, David Luecke notifications@github.com wrote:

I can see that this might happen. If it is as easy as adding if(!this.element) return; before https://github.com/bitovi/canjs/blob/master/control/control.js#L702 I don't see why it shouldn't be fixed.


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Apr 18, 2013

I wanted this error in the past because it meant I was doing something else wrong.

Curtis, can you share some code that should be fine that creates this error.

Sent from my iPhone

On Apr 18, 2013, at 9:53 AM, David Luecke notifications@github.com wrote:

I can see that this might happen. If it is as easy as adding if(!this.element) return; before https://github.com/bitovi/canjs/blob/master/control/control.js#L702 I don't see why it shouldn't be fixed.


Reply to this email directly or view it on GitHub.

@ccummings

This comment has been minimized.

Show comment
Hide comment
@ccummings

ccummings Apr 19, 2013

Contributor

When you encounter this error, it doesn't necessarily mean you've done something wrong. Take this fiddle for instance.

If you call destroy on the parent control, you would want it to destroy the child control instance as well. But if the parent control's element is removed from the DOM, this same logic will now throw an error.

We could console.warn that the instance was already destroyed, but I don't think a JavaScript error should be thrown in this case.

Contributor

ccummings commented Apr 19, 2013

When you encounter this error, it doesn't necessarily mean you've done something wrong. Take this fiddle for instance.

If you call destroy on the parent control, you would want it to destroy the child control instance as well. But if the parent control's element is removed from the DOM, this same logic will now throw an error.

We could console.warn that the instance was already destroyed, but I don't think a JavaScript error should be thrown in this case.

ccummings added a commit that referenced this pull request May 1, 2013

Merge pull request #352 from bitovi/issue-352
Prevent errors if destroy is called multiple times on a Control instance

@ccummings ccummings merged commit 41f0c21 into master May 1, 2013

1 check passed

default The Travis build passed
Details

@ccummings ccummings deleted the issue-352 branch May 1, 2013

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