Unbind uiScrollfix directive jQuery event handlers to prevent memory leaks #124
Conversation
The uiScrollfix directive will bind scroll event handlers to the window object in the absence of a parent uiScrollfixTarget directive. These event handlers were not removed when the directive was destroyed, causing a memory leak. We prevent this by removing bound jQuery scroll event handlers when the scope is $destroyed. Unit tests have been updated to verify these events are correctly removed. Resolves #119
There was a problem hiding this comment.
@ajpiano validated my concerns about using $._data here—I will update this PR with a patch to leverage spies and mocks rather than relying on a private jQuery API.
|
Awesome, that's way more reliable then. I don't think AngularJS bindings support namespacing events anyway so that's a nifty trick. I just learned (or re-learned, cuz I'm sure I knew that already) something lol. Only note is try to refactor your tests so that they don't rely on jQuery as this repo is supposed to have 0 dependencies on it. I realize certain positional calculations of the other modules could benefit from jQuery's |
Unbind uiScrollfix directive jQuery event handlers to prevent memory leaks
|
My laptop died so I didn't get a chance to revisit those tests. Will try to work on that soon and submit a new PR to remove the dependence on |
|
@kadamwhite I would checkout the discussion on #123 |
|
Interesting! @ProLoser noted, I'll catch up on the discussion. In the meantime, I did find a way to test this without _data: https://github.com/kadamwhite/ui-utils/compare/issue;119-scrollfix-memory-leak Looks like a PR wouldn't be useful since this may be going away, but I can send if desired. |
|
Might as well, but you may want to read my newest comment as it proposes an even MORE efficient approach than the one used here. Alternatively, if you don't want to waste the effort (which is fine too) we can just collaborate on the other issue if it looks promising to you. |
2 participants
The
uiScrollfixdirective will bind scroll event handlers to the window object in the absence of a parentuiScrollfixTargetdirective. These event handlers were not removed when the directive was destroyed.We prevent this by removing bound jQuery scroll event handlers when the scope is
$destroyed. Tests have been updated to verify events are correctly removed.Resolves #119