Unpaired bindEvent/unbindEvent with reload()

We just found out that a call to reload() does not unbind the resize/scroll events, while the events are bound again when the data is loaded.
Steps to reproduce:

Launch Reload100 demo app
Open your JS debugger and put a breakpoint on both bindEvents() & unbindEvents()
Click the reload button from the demo page. The debugger breaks on bindEvents but not on unbindEvents

Possible solutions:

add a call to unbindEvent() within reload
keep a flag defining if the events are already bound like bellow:

        let eventBound = false;
        function bindEvents() {
          if(!eventBound) {
            eventBound = true;
            viewport.bind('resize', resizeAndScrollHandler);
            viewport.bind('scroll', resizeAndScrollHandler);          
          }
        }
        function unbindEvents() {
          if(eventBound) {
            eventBound = false;
            viewport.unbind('resize', resizeAndScrollHandler);
            viewport.unbind('scroll', resizeAndScrollHandler);
          }
        }

@priandsf

@priandsf Thank you for the contribution! Let's see what's going on here... I made some test of 3 series of 5 reloads:

start profiling
reload x5, GC
reload x5, GC
reload x5, GC
stop profiling

The result is below

You see, a number of Listeners does not increase. Also, I don't see increasing number of "scroll" listeners via getEventListeners and Event Listeners inspector. Assigning the same function as a new listener must not lead to duplication. That's for example how addEventListener works:

If multiple identical EventListeners are registered on the same EventTarget with the same parameters, the duplicate instances are discarded.

But we are using jQlite's .bind method and looks like there is some magic inside which do make duplicates which are invisible for Chrome Dev Tools inspector. I have no other explanation. Break points show us the invisible duplicated listeners do work.

We don't see any side-effects due to rareness of the reload calls and due to some conditional protections in the ui-scroll code (expensive logic don't being executed in the result of duplicate listeners triggering). But I agree with you, this should be fixed.

As a solution I would prefer to have unbindEvents() call in the beginning of the reload method. Would you like to open a PR on 'master'? I'll merge it quickly.

Thanks! I created the pull request #227

I agree with your findings, although we had a different use case. Because the directive attributes are not observed (and it is ok), we have to fully reset the directive (delete/recreate) when we want to change one of them, namely the new rowHeight we added. Doing that leaves some handlers attached which creates a memory leak, and makes a dead directive still handling the event.

@priandsf

@priandsf The PR is merged, I'm going to make a release in these 2 days.

angular-ui-scroll v1.7.5 have been released

Navigation Menu

Repository navigation

Unpaired bindEvent/unbindEvent with reload() #226

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Unpaired bindEvent/unbindEvent with reload() #226

Description

Activity

dhilt commented on Jul 30, 2019

priandsf commented on Jul 31, 2019

dhilt commented on Jul 31, 2019

dhilt commented on Aug 3, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions