lorenzofox3 / lrInfiniteScroll

directive to register handler when an element is scrolled down near its end http://lorenzofox3.github.io/lrInfiniteScroll/index.html
217 stars 46 forks source link

Remove event handler when scope is destroyed #12

Closed prismsoundzero closed 8 years ago

prismsoundzero commented 8 years ago

Make sure that the event handlers attached to this element are removed when the element is removed from the DOM

lorenzofox3 commented 8 years ago

I don't think it is necessary as the the element you bind the event handler to is the element which holds the directive, Angular takes care of the cleaning. Also if I am not wrong unbind will remove all the listeners not only the one set by infinite scroll which can be problematic if there are other directives

prismsoundzero commented 8 years ago

What I am understanding from the AngularJS' doc is that AngularJS does not take care of the cleaning of event handlers "manually" attached to an element (i.e. without AngularJS core directives).

https://docs.angularjs.org/api/ng/type/$rootScope.Scope#$destroy

Just before a scope is destroyed, a $destroy event is broadcasted on this scope. Application code can register a $destroy event handler that will give it a chance to perform any necessary cleanup. Note that, in AngularJS, there is also a $destroy jQuery event, which can be used to clean up DOM bindings before an element is removed from the DOM.

But you are right, unbind without any argument removes all the listeners so it might be better to unbind only the scroll event.

lorenzofox3 commented 8 years ago

Yes, it refers to the case where you bind to other elements. For example if in your directive you add an event listener to the window object and don't clean when the scope is destroyed: the element won't be garbage collected as it points to a remaining element (window) and you'll have a memory leak. However if the event listener remains on the element which is destroyed you will not have any issue and the element will be garbage collected (well depends what it has in its closure too, but that is a different story) as it does not hold any reference to the rest of the memory tree

https://github.com/lorenzofox3/stStickyHeader/blob/master/stStickyHeader.js an example of a directive that need to be cleaned.

Have you experienced memory leak with lrInfiniteScroll ?

prismsoundzero commented 8 years ago

OK thanks, it makes sense.

No, I did not experience memory leak with lrInfiniteScroll.