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

useAsyncFn - return Promise.reject() or Promise.resolve() in the promise resolution #612

Closed
wants to merge 3 commits into from

Conversation

diegoarcega
Copy link

Problem

const [registration, register] = useAsyncFn(Service.register)

async function handleSubmit(){
  try {
    await register(payload)
    // it always succeeds
  } catch(error){
    // it never reaches here
  } 
}

@wardoost
Copy link
Contributor

I think you want to do the following:

const [{value: register, loading, error}, handleRegisterSubmit] = useAsyncFn(
  () => Service.register(payload),
  [payload]
)

That said, I'm not entirely clear on why the callback returns the value or error in the first place...

@xobotyi
Copy link
Contributor

xobotyi commented Sep 18, 2019

@wardoost because, as i remember, if value returned from then callback is not a promise - it is automatically wraped with fulfilled promise.

@diegoarcega
Copy link
Author

@wardoost what you suggested above gave me the same result, can't reach the catch(error) part of the code. It is impossible to handle an error with the given implementation of useAsyncFn.

@wardoost
Copy link
Contributor

wardoost commented Sep 20, 2019

I see what you mean now. I would have implemented your example like this as I would want the state represent how the async function resolved or failed:

const [{value, loading, error}, handleSubmit] = useAsyncFn(
  async () => {
    try {
      const result = await Service.register(payload)
      // handle result
    } catch (error) {
      // handle error
    }
  },
  [payload]
)

I can see there are use cases where you want the returned state of this hook to only represent part of some async function or handle failure differently with separate state. Your change seems to do this well but an error is show in the console when not catching errors which this hook is intended to do. I would recommend using this hook as I show above.

@diegoarcega
Copy link
Author

@wardoost I see you grasped the idea, and the way you've shown above solves the problem if I only call handleSubmit once, which I think suits most use cases. However, if I reuse this handleSubmit in other parts of the code and want to handle errors differently depending on where handleSubmit was called, I cant do because I would only have one implementation of the catch block.

I've changed useAsync code a little bit
image

Now it won't complain about not handling error thrown on useAsyncFn's promise. Wdyt?

@diegoarcega diegoarcega changed the title Return Promise.reject() or Promise.resolve() in the promise resolution useAsyncFn - return Promise.reject() or Promise.resolve() in the promise resolution Sep 21, 2019
@wardoost
Copy link
Contributor

@diegoarcega This only solves the the problem of uncatched errors in useAsync, the problem still persists with useAsyncFn.

As you've mentioned my snippet above solves most use cases. The problem you're trying to solve seems to be the edge case which could be solved by checking if the awaited result is an error. If it is an error object you decide what to do with it, which can be handled differently in different places.

@diegoarcega
Copy link
Author

diegoarcega commented Sep 22, 2019

@wardoost

I understand that with useAsync one wouldn't want to handle errors in the callback, because this is only called once on component mount, no need to manually call the callback. However, with useAsyncFn an error can occur at any point while interacting with the app and the developer will want to not only show a message when the useAsyncFn returns an error, but also maybe make another request to the server or any other non UI related action.

Moreover, I wouldn't say catching an error after a failed promise is an edge case, it is more of a normal routine in software development. If an error occurs, I would expect the app to have a handler for that.

Checking for error in the success response doesn't seem like how standard apis should be made, more like a workaround imo.

One of the reasons why one would use a useasyncfn hook is its reusability but if the dev wants to reuse the callback in just one more place, it won't work, a new callback needs to be created but then you will be repeating the creation of the callback just because you are using it in another place and the hook doesnt support that.

I see that useAsyncFn was designed to handle simpler cases and it is not the idea of this hook to implement what I said above. I will have to create another hook with the features I want. Thanks @wardoost

@wardoost
Copy link
Contributor

wardoost commented Oct 5, 2019

@diegoarcega Catching an error after a failed promise is definitely not an edge case.. That's why errors are catched in all async hooks in this library and expressed in an object with loading, error and value keys to represent the async state of the promise in a declarative way. If you want to display an error message your would check the error key of the async state and display an error message accordingly as shown in the useAsyncFn docs.

You can reuse the callback in different places in your components but if a promise handles errors differently, and maybe fire off a second network request when that happens, it is not the same callback.

Checking for error in the success response doesn't seem like how standard apis should be made, more like a workaround imo.

I definitely agree on this hence why I said the following in my first comment:

That said, I'm not entirely clear on why the callback returns the value or error in the first place...

In my opinion the callback shouldn't return anything as to not confuse devs. It looks like it was put there as an escape hatch to do things you are trying to do.

@wardoost wardoost closed this Oct 5, 2019
@mengxiong10
Copy link

Why not throw the error ?

@mqliutie
Copy link

mqliutie commented Aug 3, 2020

I can't catch the error... I must check if the response is instance of Error... sad

@diegoarcega diegoarcega deleted the fix/return-promise branch August 3, 2020 12:12
@chomamateusz
Copy link

Guys, I had the same problem and the quick workaround form me was introducing a new hook in my project to replace useAsyncRetry:

import React from 'react'

import { useAsyncFn as useAsyncFnOriginal } from 'react-use'

export const useAsyncFn = (...args: Parameters<typeof useAsyncFnOriginal>): ReturnType<typeof useAsyncFnOriginal> => {
  const [state, callback] = useAsyncFnOriginal(...args)

  const callbackEnhanced = React.useCallback(async (...cbArgs: Parameters<typeof callback>) => {
    const result = await callback(...cbArgs)
    if(result instanceof Error) throw result
    return result
  }, [callback])

  return [state, callbackEnhanced]
}

But will it only work when the promise will reject with an error!

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

Successfully merging this pull request may close these issues.

None yet

6 participants