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

Memory leak: scroll event listener not being removed #141

Closed
abulrim opened this issue Apr 5, 2018 · 2 comments · Fixed by #138
Closed

Memory leak: scroll event listener not being removed #141

abulrim opened this issue Apr 5, 2018 · 2 comments · Fixed by #138
Labels

Comments

@abulrim
Copy link

abulrim commented Apr 5, 2018

Version

3.0.0

Steps to reproduce

  • Enter route that has a component that uses in-viewport mixin on window
  • Leave to route that doesn't have that component
  • Check event listeners on window in the dev tools
  • Notice scroll event still attached

Expected Behavior

Scroll events should be cleaned when element destroyed

Likely Culprit

The changes done to the _unbindListeners method in the in-viewport mixin in this PR (we return before reaching this._unbindScrollDirectionListener())

@abulrim
Copy link
Author

abulrim commented Apr 5, 2018

Also removing the event listener in the current implementation won't work since the callback is a new function each time

@snewcomer
Copy link
Collaborator

snewcomer commented Apr 5, 2018

@abulrim Great catch! The early return is my fault. Oversight by me. The second point - So we are passing in an anon callback; however, I'm also seeing this as I would only guess it is not a reference to the function object.

If you felt like submitting a PR to use Function.prototype.bind for both the scroll event listeners and the viewportListeners (or other solution you have), that would be great! Otherwise, I can push something up to fix these as well. Let me know! Again, thanks for bringing this up and documenting it so well.

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

Successfully merging a pull request may close this issue.

2 participants