mousetrap
mousetrap copied to clipboard
ccampbell
Add Mousetrap.prototype.destroy
Why isn't this merged already? =) When you can create new instances, you should be able to remove them!
+1
I was investigating a memory leak in a react app which lead me here. react-hotkeys uses this lib and naively calls reset() assuming things will be cleared up but they aren't.
This means that every time your react component is mounted or rerendered it will leak 3 event handlers.
Well it looks like this has been forgotten, but i dont like idea of generating memory leaks, that means Mousetrap is not usable for scoped events without memory leaks so far, pity.
It is only usable globaly for document
@ccampbell any chance we could get this merged? mousetrap looks great for our use case but the inability to properly unbind the event listener it creates is a show-stopper for us: our use case is an application that can be unmounted and re-mounted, resulting in event listeners piling up.
Hi. Thanks for following up on this. I am happy to merge this.
My only question for @bolasblack (if he even remembers after all these years) is why the destroy method was omitted from the global Mousetrap instance (the one on document)? I can’t think of a reason to prevent someone from using the global Mousetrap instance and then calling Mousetrap.destroy() to remove the event listeners on document.
Yeah, I ran into that too when trying to actually call destroy() from application code. Seems like that one change should not be in this diff.
Hi guys.
Well i was trying to find out whether is that missing destroy big problem or not. As i said before, its problem when you want to have global shortcuts and also local lets say for form. But as i found out, all event listeners are garbage collected by browser if there are no references on any instance that uses them. So maybe:
this._mousetrap.reset();
this._mousetrap = null;
is enough. But i did not try to profile it, so I am not sure.