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

feat: improve useAsyncFn and useAsync typings #589

Merged
merged 4 commits into from Dec 30, 2019
Merged

Conversation

xobotyi
Copy link
Contributor

@xobotyi xobotyi commented Sep 4, 2019

useAsyncFn and useAsync typings made great again =)
Now it uses fully generic arguments and returning types, thus no problems with inferring callback arguments and value types;

PS: @streamich check the #381 PR too.

Now it uses fully generic arguments and returning types, thus no problems with inferring callback call arguments;
@xobotyi xobotyi added the enhancement New feature or request label Sep 4, 2019
Fix function name;
@fabiancook
Copy link

Hi, I think these types have been made a bit too complex, I'll expand on this tomorrow, just leaving this here to flag the merging of it

@xobotyi
Copy link
Contributor Author

xobotyi commented Sep 6, 2019

@fabiancook any results on making it simpler?

@fabiancook
Copy link

My main concern is that an unneeded type is being used.

Instead of

export type AsyncFnReturn<T extends FnReturningPromise = FnReturningPromise> = [AsyncState<ReturnType<T>>, T];

You should have

export type AsyncReturnFn<T, P extends any[]> = [AsyncState<T>, (...args: P) => Promise<T>];

This allows the parameters to be passed, along with the return type, removing the need for ReturnType to be used

@xobotyi
Copy link
Contributor Author

xobotyi commented Sep 7, 2019

It is made this way now and has problems with returned function arguments.
Mine approach infers function type automatically and has no need to re-specify function's arguments to make it work.

@ranisalt
Copy link

ranisalt commented Oct 1, 2019

What is pending? This is cumbersome for a very common use case, form submission:

  const [state, submit] = useAsyncFn(async (e: React.FormEvent) => {
    e.preventDefault()
    // submit form
  })
  return (
    <form onSubmit={submit} {...props}>
      {children}
    </form>
  )

Currently I must wrap it on site for onSubmit, and worse yet I must access the form other way as I can't use the event target!

  const [state, submit] = useAsyncFn(async () => {
    // submit form
  })
  const formRef = useRef<HTMLFormElement>(null)
  return (
    <form ref={formRef} onSubmit={async e => {
      e.preventDefault()
      return submit()
    } {...props}>
      {children}
    </form>
  )

@pnarielwala-tc
Copy link

This would really be helpful if this is fixed 🙂. Below is what I'm having to do in the meantime

interface ApiRequestData {}
interface ApiResponse {}

const [state, apiCall] = useAsyncFn<
  AxiosResponse<ApiResponse>,
  [ApiRequestData]
>((...args) => {
  const data = args[0] // 😩 
  return axios.post('/api/call', data)
})

while what I want to do is

const [state, apiCall] = useAsyncFn<
  AxiosResponse<ApiResponse>,
  ApiRequestData // error here
>((data: ApiRequestData) => {
  return axios.post('/api/call', data)
})
/* error
Type 'ApiRequestData' does not satisfy the constraint 'any[]'.
  Type 'ApiRequestData' is missing the following properties from type 'any[]': length, pop, push, concat, and 28 more.
*/

And when I try to fix that error, I get

const [state, apiCall] = useAsyncFn<
  AxiosResponse<ApiResponse>,
  [ApiRequestData]
>((data: ApiRequestData) => { // error here
  return axios.post('/api/call', data)
})
/* error
Argument of type '(data: ApiRequestData) => Promise<AxiosResponse<ApiResponse>>' is not assignable to parameter of type '(...args: [ApiRequestData] | []) => Promise<AxiosResponse<ApiResponse>>'.
  Types of parameters 'data' and 'args' are incompatible.
    Type '[ApiRequestData] | []' is not assignable to type '[ApiRequestData]'.
      Property '0' is missing in type '[]' but required in type '[ApiRequestData]'.
*/

@xobotyi
Copy link
Contributor Author

xobotyi commented Oct 13, 2019

@pnarielwala-tc dows this pr solve your problem? if yes - i'll merge it.

@pnarielwala-tc
Copy link

pnarielwala-tc commented Oct 14, 2019

@xobotyi so unfortunately I ran some minor issues as I pulled your change locally into my codebase. I was also getting eslinting issues with the dep argument so I ended up removing it since I didn't see a use case of needing it. We would really only need the useCallback to update if fn ever changed so made that a dependency directly This is what I did to get it working

import { useCallback, useState } from 'react'
import { useMountedState } from 'react-use'

export type FnReturningPromise = (...args: any[]) => Promise<any>

export type PromiseType<P extends Promise<any>> = P extends Promise<infer T>
  ? T
  : never

export type AsyncState<T> =
  | {
      loading: boolean
      error?: undefined
      value?: undefined
    }
  | {
      loading: false
      error: Error
      value?: undefined
    }
  | {
      loading: false
      error?: undefined
      value: T
    }

type StateFromFnReturningPromise<T extends FnReturningPromise> = AsyncState<
  PromiseType<ReturnType<T>>
>

export type AsyncFnReturn<T extends FnReturningPromise = FnReturningPromise> = [
  StateFromFnReturningPromise<T>,
  T,
]

export default function useAsyncFn<T extends FnReturningPromise>(
  fn: T,
  initialState: StateFromFnReturningPromise<T> = { loading: false },
): AsyncFnReturn<T> {
  const isMounted = useMountedState()
  const [state, set] = useState<StateFromFnReturningPromise<T>>(initialState)

  const callback = useCallback(
    (...args: Parameters<T>): ReturnType<T> => {
      isMounted() && set({ loading: true }) // for when the component using this hook unmounts for some reason

      return fn(...args).then(
        value => {
          isMounted() && set({ value, loading: false })

          return value
        },
        error => {
          isMounted() && set({ error, loading: false })

          return error
        },
      ) as ReturnType<T>
    },
    [fn, isMounted], // replaced `deps` here as I didn't have a real use case for a dependency list
    // also when this was just `deps` (as React.DependecyList), I was getting warnings listed below
  )

  return [state, (callback as unknown) as T]
}

eslint warnings

React Hook useCallback was passed a dependency list that is not an array literal. This means we can't statically verify whether you've passed the correct dependencies 
React Hook useCallback has missing dependencies: 'fn' and 'isMounted'. Either include them or remove the dependency array. If 'fn' changes too often, find the parent component that defines it and wrap that definition in useCallback

@xobotyi xobotyi force-pushed the master branch 16 times, most recently from 95c7eb3 to 361af9a Compare November 3, 2019 03:21
@xobotyi xobotyi force-pushed the master branch 17 times, most recently from 4baf4c2 to 7e57723 Compare November 6, 2019 20:49
@xialvjun
Copy link

What is pending?

@streamich streamich changed the base branch from master to v14.0 December 30, 2019 23:05
Copy link
Owner

@streamich streamich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@streamich streamich merged commit 85967e2 into v14.0 Dec 30, 2019
@streamich streamich deleted the fix-useAsync-types branch December 30, 2019 23:09
@gavinxgu
Copy link

I am also struggling with this problem, when will v14.0 release? Thanks!

@LeoAnesi
Copy link

LeoAnesi commented Apr 6, 2020

Thanks a lot for that PR @xobotyi, thanks to you I'm gonna remove most of the ts-ignore of my codebase :)

@maku693 maku693 mentioned this pull request May 13, 2020
@streamich
Copy link
Owner

🎉 This PR is included in version 15.0.0 🎉

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
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants