search

davatron5000 / FitText.js

A jQuery plugin for inflating web type
http://fittextjs.com
6.76k stars 1.39k forks source link

No destroy method == memory leak #135

Closed JSteunou closed 9 years ago

JSteunou commented 9 years ago

there is a window.on but no off

you should expose a destroy method that unbind event listener.

davatron5000 commented 9 years ago

Closed due to no reduced test case.

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

JSteunou commented 9 years ago

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

davatron5000 commented 9 years ago

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

JSteunou commented 9 years ago

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 commented 9 years ago

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

Petah commented 9 years ago

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

FezVrasta commented 8 years ago

+1

JSteunou commented 8 years ago

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

davatron5000 commented 8 years ago

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

FezVrasta commented 8 years ago

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

@davatron5000 thanks for the effort

Petah commented 8 years ago

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

davatron5000 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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