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

No destroy method == memory leak #135

Closed
JSteunou opened this issue May 22, 2015 · 14 comments · May be fixed by #136 or #150
Closed

No destroy method == memory leak #135

JSteunou opened this issue May 22, 2015 · 14 comments · May be fixed by #136 or #150

Comments

@JSteunou
Copy link

there is a window.on but no off

you should expose a destroy method that unbind event listener.

@davatron5000
Copy link
Owner

Closed due to no reduced test case.

http://api.jquery.com/off/

@JSteunou
Copy link
Author

so this plugin does a .on but it's on user to call .off ?

@davatron5000
Copy link
Owner

Yeah, if it's something you need to turn off you can totally make that happen.

@JSteunou
Copy link
Author

So I would write several off in my code, but if someday you change your events or add some, I would rewrite my code.

Good practice is to expose a destroy method.

@Petah
Copy link

Petah commented Sep 2, 2015

+1 on a single page app with many elements being created and destroyed this does leak memory, and reduce performance.

@Petah
Copy link

Petah commented Sep 2, 2015

Also we can't manually call off because the event function is not exposed to the app scope.

@FezVrasta
Copy link

+1

@JSteunou
Copy link
Author

JSteunou commented Jan 5, 2016

@FezVrasta fork it @davatron5000 is not open to it and not maintaining it anymore...

@davatron5000
Copy link
Owner

I'm not opposed. Just busy. Will try to look at it today after a few @a11yproject things. 

@FezVrasta
Copy link

@JSteunou I've already did it, unfortunately I need some more advanced feature.

@davatron5000 thanks for the effort

@Petah
Copy link

Petah commented Jan 5, 2016

Not sure why the issue is closed though, as it clearly is an issue.

@davatron5000
Copy link
Owner

@Petah It was closed due no reduced test case per the contributing.md. I'll take a look at #136 but I can't just merge to master something that says "Should fix some issue" without vetting it first. No reduced test case means I have to take more of my time to get that integrated.

Also we can't manually call off because the event function is not exposed to the app scope.

I don't have a single line of code from you explaining what you're trying to do. I can't tell if this is super unique to your implementation or not. I don't know if you've rolled out @JSteunou's fork and if that has worked for you. I literally don't know anything at this point other than a few +1's.

If I could get concrete feedback from anyone on this thread as opposed to pejorative quips, that would be much appreciated and I'll consider reopening.

@FezVrasta
Copy link

Ok, you bind an event with

$(window).on('resize', myFn);

you instead have to:

var myFnBound = myFn.bind(arg1, arg2, arg3);
$(window).on('resize', myFnBound);

and in the destroy method:

$(window).off('resize', myFnBound);

@Petah
Copy link

Petah commented Jan 5, 2016

I don't think its really possible to create a test case for not being able to unbind an event.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants