streamich / react-use

React Hooks — 👍
http://streamich.github.io/react-use
The Unlicense
42.01k stars 3.16k forks source link

`createMemo` does not support variadic functions #2294

Open bug-brain opened 2 years ago

bug-brain commented 2 years ago

What is the current behavior? When feeding a variadic function into createMemo and calling the result with different numbers of arguments React will log an error on the console: https://codesandbox.io/s/eloquent-poincare-2vmcq?file=/src/App.js.

What is the expected behavior? createMemo does not produce errors when the number of arguments changes.

A little about versions:

juliolmuller commented 2 years ago

I would say it's a bad practice to change the number of arguments being passed to the function. If you take a look at the implementation of createMemo factory, it uses the arguments passed to the custom hook as dependency array to useMemo hook. Therefore, the warning displayed in the console is React's default, as providing computed dependency arrays are discouraged. Keeping the same number of arguments is typically expected, so changing it will make React raise a flag that something unexpected is happening.

If using the native hooks directly, like useEffect, useCallbackoruseMemo, ESlint would typically complain with thereact-hooks/exhaustive-deps` rule, but as it's wrapped by another abstraction, this bad practice is hidden.

So, @bug-brain, I would say there is not much that can be done here. Anyway, the message in the console is just a warning. I'm quite sure it doesn't show in production, does it?

bug-brain commented 2 years ago

In my opinion arguments and dependencies are different things but I agree the number of arguments should remain the same in the vast majority of cases. In my use-case a variable number of chunks had to be processed if a pairwise === check failed for which createMemo looked promising. Maybe you could add a note to the docs that says the arguments are passed to useMemo and thus the usual rules apply.

juliolmuller commented 2 years ago

In my opinion arguments and dependencies are different things but I agree the number of arguments should remain the same in the vast majority of cases. In my use-case a variable number of chunks had to be processed if a pairwise === check failed for which createMemo looked promising. Maybe you could add a note to the docs that says the arguments are passed to useMemo and thus the usual rules apply.

I think it's a nice approach. That would allow the user to be aware of this behavior more easily.