Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Given function for useDebounce should expect arguments (as for the type signature) #618

Closed
minagawah opened this issue Sep 23, 2019 · 5 comments · Fixed by #623
Closed
Labels

Comments

@minagawah
Copy link

What is the current behavior?

Although the first argument of useDebounce (which is a function) expects arguments, the type signature for the function does not seem to accept arguments, and I get the following TypeScript error:

Argument of type '(screen: any) => any' is not assignable to parameter of type '() => any'.  TS2345

While the actual implementation (within useDebounce) is no problem, I suggest to change the signature to take arguments:

-const useDebounce = (fn: () => any, ms: number = 0, args: any[] = []) => {
+const useDebounce = (fn: (...args: any[]) => any, ms: number = 0, args: any[] = []) => {

Steps to reproduce it and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have extra dependencies other than react-use. Paste the link to your JSFiddle or CodeSandbox example below:

const [screenSize, setScreenSize] = useState({ width: 0, height: 0 });

const resize = (screen): any => {
  const { width = 0, height = 0 } = screen;
  console.log(`screen: ${width}x${height}`);
};

useDebounce(resize, 500, [screenSize]);

useEffect(() => {
  setScreenSize({
    width: window.innerWidth || 0,
    height: window.innerHeight || 0,
  });
}, []);

What is the expected behavior?

It should not raise a TypeScript error even if I specify arguments to the function.

A little about versions:

  • OS: 16.04.1-Ubuntu
  • Browser (vendor and version): Chrome Version 77.0.3865.75 (Official Build) (64-bit)
  • React: 16.9.0
  • react-use: 12.2.0
@blasterpistol
Copy link

blasterpistol commented Sep 24, 2019

I think args are not actually function args, they are deps, if deps changing so debounce function called. It's why you should directly use the screenSize variable inside the resize function.

// ...

const resize = () => {
  const { width = 0, height = 0 } = screenSize;
  console.log(`screen: ${width}x${height}`);
};

// ...

@minagawah
Copy link
Author

I completely understand. However, as the code within the function is defined, it seems to pass dependencies as function arguments.

const handle = setTimeout(fn.bind(null, args), ms);

So, we have 2 choices to either:

  1. Fix the type signature for the arguments for TypeScript users, or
  2. Fix the actual codes NOT to pass the arguments.

If we were to strictly follow the Hooks rule, yes, @testarossaaaaa , I agree with you.
That we should directly use screenSize within the handler, and should not pass dependencies as arguments, which leads us to choose (2).
But, some developers may have their codes using the arguments already, and as we should avoid breaking their codes, we probably should simply add the type signature... which leaves us choosing (1).

It works just fine with plain Javascript, but not for TypeScript... which sometime annoys me, and makes me reconsider if it is worth to define types at all...

@wardoost
Copy link
Contributor

wardoost commented Sep 25, 2019

@minagawah I think we should remove the dependency array from the function arguments as standard React hooks don't do this either. Currently the deps array is also not spread (fn.bind(null, ...args)) which makes me believe that there will be very few developers actually using the timeout function arguments as you would actually have to destructure:

const resize = ([screen]): any => {
  const { width = 0, height = 0 } = screen;
  console.log(`screen: ${width}x${height}`);
};

I have opened a PR that implements these changes, let me know what you think.

@minagawah
Copy link
Author

@wardoost PR looks good. And, yes. You are so right about the spreading as well.
I hope it merges soon.
BTW, I personally don't prefer the current standard hooks rule as it should allow passing arguments around for they are obviously not pure functions at all....

@streamich
Copy link
Owner

🎉 This issue has been resolved in version 12.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants