Please for the love of god, do not memoize our callbacks for us.

[garylli]

You're introducing non-react-like update behavior to all users, not as an opt-in, but as a default. Any new programmers to the codebase will see the hook and assume it behaves in a react-like way.

A hook should not obscure the react update cycle. It makes it harder to use and harder to understand. By memoizing the callback IN the component rather than in user land, you've created a side effect that is extremely hard to track.

Like the react docs state, hooks and components should be idempotent. When I pass the same callback to the hook, it should exhibit the same behavior.

Yet, with the current hook, I can pass the same callback in and get two different behaviors because you've decided to inadvertently create stale states.

[JohannesKlauss]

True, because not memoising it will cause rerenders all over the place and you'll have to memoise it yourself 99,9% of the time. And with the coming introduction of the react compiler, this whole step will become obsolet anyways 🙂

[garylli]

You're correct in that you will have to memoize it yourself 99.9% of the time.

But that's the expected behavior.
When someone declares a local function, it's expected to recalculate on rerender unless it's memoized.

const SomeComponent: FC = () => {
    // We expect these values to be recalculated on every rerender.
    const someCondition = someNum > 0;
    const callbackFn = () => { }
    
    return (
       // updates with the value of callbackFn as expected.
       <SomeChildComponent
          callback={callbackFn}
       >
    )
}

When you memoize it within the hook, that expected behavior breaks.

const SomeComponent: FC = () => {
    // We expect these values to be recalculated on every rerender.
    const someCondition = someNum > 0;
    const callbackFn = () => { }
    
    // does NOT update with the value of callbackFn even though it looks like it should.
    useKeybinds('f', callbackFn);
    
    return (
       // updates with the value of callbackFn as expected.
       <SomeChildComponent
          callback={callbackFn}
       >
    )
}

I'm not caught up with the react compiler changes, so if what you say is true, then this discussion will become obsolete soon anyway. But it's something to keep in mind when publishing react packages.

Thanks for the work you've done on the package and also for the reply.

I was a bit frustrated because a teammate spent hours debugging a component due to the aforementioned behavior so I do apologize for the tone of the previous post. That's never the correct way to start a discussion.

[JohannesKlauss]

Agreed. I know that this breaks with intended behavior, on the other hand you're the first case in 4 or 5 years since this hook exists, so it kind of proves the point of the exception. And reverting it now would be a massive breaking change, because so many rely on the current behavior of the hook. So it's not that easy to just drop it 😉

Apologies accepted, I probably should add a disclaimer that explains the memoisation.