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

useLoadable #44

Open
hanford opened this issue Oct 29, 2018 · 8 comments
Open

useLoadable #44

hanford opened this issue Oct 29, 2018 · 8 comments

Comments

@hanford
Copy link
Member

hanford commented Oct 29, 2018

I love how GraphQL <QueryRender /> have render props that return

({ loading, props, error, retry })

I often find myself writing the same logic around form submit buttons and other various UI where I need to know whether my async function is loading, had an error or the result.

I'm proposing an API like this:

const sleep = time => () => new Promise(done => setTimeout(done, time));

function App() {
  const [onClick, { loading, error }] = useAsyncFunction(sleep(500));

  return (
    <>
      <pre>
        error: {JSON.stringify(error)}
        <br />
        loading: {JSON.stringify(loading)}
      </pre>
      <button onClick={onClick}>{loading ? "Loading..." : "Load"}</button>
    </>
  );
}

I think we should also support a delayMs option that will tell our async function only resolve after a given ms .. it should work like this:

const sleep = () => Promise.resolve();

function App() {
  const [onClick, { loading, error }] = useAsyncFunction(sleep, { delayMs: 500 });

  return (
    <>
      <pre>
        error: {JSON.stringify(error)}
        <br />
        loading: {JSON.stringify(loading)}
      </pre>
      <button onClick={onClick}>{loading ? "Loading..." : "Load"}</button>
    </>
  );
}

I have most of the code working, I'll just need to add type defintions and some better testing in this repo: https://github.com/hanford/async-function

Once that's done, I'll transfer the ownership (aiming to be complete tomorrow morning)

@j-f1
Copy link

j-f1 commented Oct 29, 2018

See also #24

@hanford hanford mentioned this issue Oct 29, 2018
@hanford
Copy link
Member Author

hanford commented Oct 29, 2018

This is done, I can transfer ownership but want to hear what the thoughts are in #24 first.

@hanford
Copy link
Member Author

hanford commented Oct 30, 2018

cc @jamiebuilds @fouad @jamesplease thoughts?

@fouad
Copy link
Member

fouad commented Oct 31, 2018

I like this but useAsync might be a bit too generic. I'd be concerned about a breaking change if Suspense has a more recommended way of handling async/await-like suspension in hooks. It reminds me of the react-loadable so I'd suggest two changes:

1. Rename to useLoadable

There are a few other react-loadable types that would be nice, e.g. loaded and timedOut in output & timeout in options

2. Output order should be [{ loading, loaded, error }, onLoad] like useState

I can understand [trigger, value] as the rationale for the current ordering but I think it might be more confusing than helpful, e.g. node libraries that switched around order for cb and err

open to pushback/other ideas!

@hanford
Copy link
Member Author

hanford commented Oct 31, 2018

I'll rename to loadable, I also think switching the order is a good idea 🙌

I'm a little on the fence about loaded (seems easy to infer from loading, also afraid of assuming people will only want to run once) and timeout because it can easily be added if desired.

It makes me think that adding some separate hooks might be better than increasing the scope of this one.

anyways - appreciate the feedback and will update this thread once I've updated the code on my end!

@fouad
Copy link
Member

fouad commented Oct 31, 2018

np! maybe @jamiebuilds will have a useful opinion on loaded vs other options (e.g. data or value)

@hanford
Copy link
Member Author

hanford commented Oct 31, 2018

@fouad sort of torn on adding the timeout functionality to useLoadable .. If useLoadable included the option to timeout, I'd expect it to subsequently cancel whatever async function is happening and that seems like a can of worms.

I'd rather defer the module consumer add some sort of composition on their end

const [{loading, error}, trigger] = useLoader(useTimeout(fetch(..), 3000))

You can check out the code here if you're curious how it would work:
https://codesandbox.io/s/k996n20p55

I've also updated the repo with the other changes from above:
https://github.com/hanford/loadable

@hanford hanford changed the title useAsyncFunction useLoadable Oct 31, 2018
@benneq
Copy link

benneq commented Jan 21, 2019

You also need to provide boolean props like isError and isLoaded, because you can have Promises that resolve (and/or reject) with null (or undefined). So you aren't able to tell if they are "done" by looking only at data and error. And if you only check for loading === false, you cannot tell if the promise already started to run or if it has finished. And if it has finished, you cannot tell if it was successful or erroneous.

For example we have several HTTP delete requests, that don't return anything on success. Of course I could refactor our API layer to always return true, but I'd say that this is something for the hook.
Because when using a normal promise you can simply call .then(() => ...) without any arguments, and you can be sure that it was successful.

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

No branches or pull requests

4 participants