Duplicate listeners when attaching to an element

[zhouzi]

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Other... Please describe:

Current behavior

When looking into #16 I noticed in the examples that listeners attached to an element are not properly cleaned. They get duplicated so you may have a listener that's fired multiple times with old values.

That could indicate a memory leak.

Expected behavior

I was expecting to have only one listener on a given element: the current one.

Minimal reproduction of the problem with instructions

https://stackblitz.com/edit/react-ts-qmxaq3?file=App.tsx

• In the reproduction, click on the gray element
• Press "i" to increase the counter
• There's 1 more alert every time "i" is pressed and the alerts show an old value

What is the motivation / use case for changing the behavior?

Environment

Libs:
- react version: latest
- reakeys version: latest


Browser:
- [x] Chrome (desktop) version XX
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX
 
For Tooling issues:
- Node version: XX  
- Platform:  

Others:

[zhouzi]

I've been looking into this and the issue is that listeners are unbinded on the global Mousetrap instance instead of the one that was used to bind:

// in addKeys
const mousetrap = k.ref?.current ? Mousetrap(k.ref.current) : Mousetrap;
mousetrap.bind(k.keys, k.callback, k.action);

// in removeKeys:
nextKeys.forEach((s) => Mousetrap.unbind(s.keys, s.action));

[amcdnl]

Ah, so its specific to when you bind to an element?

[zhouzi]

Ah, so its specific to when you bind to an element?

Yes, otherwise all events are attached through the same global instance so that's fine. I'll suggest a fix with a PR.

[amcdnl]

That would be awesome!

[zhouzi]

There's a case I am not sure how to deal with, imagine the following:

useHotkeys([
  {
    name: 'Say hello',
    keys: ['h'],
    callback: () => console.log('hello'),
    ref: someElementRef
  },
  {
    name: 'Scream hello',
    keys: ['h'],
    callback: () => console.log('HELLO'),
    ref: someElementRef // same ref as above
  }
]);

What should pressing "h" log? hello or HELLO? I think it would make more sense that listeners from the same array overwrite each other (and thus only log HELLO). And then there's:

useHotkeys([
  {
    name: 'Say hello',
    keys: ['h'],
    callback: () => console.log('hello'),
    ref: someElementRef
  },
]);

useHotkeys([
  {
    name: 'Scream hello',
    keys: ['h'],
    callback: () => console.log('HELLO'),
    ref: someElementRef // same ref as above
  }
]);

Not sure if it should only log HELLO now, one might want to have both shortcuts work.