Skip to content

No destroy method == memory leak #135

Closed
@JSteunou

Description

@JSteunou

there is a window.on but no off

you should expose a destroy method that unbind event listener.

Activity

davatron5000

davatron5000 commented on May 22, 2015

@davatron5000
Owner

Closed due to no reduced test case.

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

JSteunou

JSteunou commented on May 22, 2015

@JSteunou
Author

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

davatron5000

davatron5000 commented on May 22, 2015

@davatron5000
Owner

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

JSteunou

JSteunou commented on May 22, 2015

@JSteunou
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

Petah commented on Sep 2, 2015

@Petah

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

Petah

Petah commented on Sep 2, 2015

@Petah

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

FezVrasta

FezVrasta commented on Jan 5, 2016

@FezVrasta

+1

JSteunou

JSteunou commented on Jan 5, 2016

@JSteunou
Author

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

davatron5000

davatron5000 commented on Jan 5, 2016

@davatron5000
Owner

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

FezVrasta

FezVrasta commented on Jan 5, 2016

@FezVrasta

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

@davatron5000 thanks for the effort

Petah

Petah commented on Jan 5, 2016

@Petah

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

davatron5000

davatron5000 commented on Jan 5, 2016

@davatron5000
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

FezVrasta commented on Jan 5, 2016

@FezVrasta

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

Petah commented on Jan 5, 2016

@Petah

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Participants

      @davatron5000@Petah@JSteunou@FezVrasta

      Issue actions

        No destroy method == memory leak · Issue #135 · davatron5000/FitText.js