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

Add useAsync hook to leverage new Hooks proposal #9

Merged
merged 22 commits into from
Dec 30, 2018
Merged

Conversation

ghengeveld
Copy link
Member

@ghengeveld ghengeveld commented Oct 29, 2018

--- This is pending test coverage ---

Usage example:

import { useAsync } from "react-async"

const promiseFn = () => fetch("/example")

const MyComponent = () => {
  const { data, error, isLoading } = useAsync({ promiseFn })
  
  // do stuff...
}

Eventually this can replace <Async> altogether:

const Async = ({ children, ...props }) => children(useAsync(props))

@codecov
Copy link

codecov bot commented Oct 29, 2018

Codecov Report

Merging #9 into master will decrease coverage by 2.08%.
The diff coverage is 97.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #9      +/-   ##
==========================================
- Coverage     100%   97.91%   -2.09%     
==========================================
  Files           1        1              
  Lines         113       48      -65     
  Branches       37       17      -20     
==========================================
- Hits          113       47      -66     
- Misses          0        1       +1
Impacted Files Coverage Δ
src/useAsync.js 97.91% <97.91%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 450aad8...73e6cb2. Read the comment docs.


const MyComponent = () => {
const { data, error, isLoading } = useAsync({ promiseFn: loadJson })
if (isLoading) return "Loading..."

Choose a reason for hiding this comment

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

Can we avoid isLoading, and use React Suspense instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Suspense is not released yet. Eventually React Async will support both. At first, React Async will maintain its API but use Suspense underneath. Later on I'll look into better integration with the Suspense components (e.g. Placeholder/Timeout). Right now there's no telling how Suspense will be used in practice.

Choose a reason for hiding this comment

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

I have to agree with @sibelius. It seems like any new api surface for async right now should at least consider the usage of resources, cache invalidation and suspense. The react team has mentioned that react-cache is a WIP and that invalidation is outstanding. Their vision seems to be using resource for data fetching. Any new api surface surface should at least be able to show how it fits into that future.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, I want React Async to be future compatible. I'm confident it will still be relevant despite Suspense offering very similar features. Personally I'm surprised that they even took the whole thing as far as providing a cache mechanism. That to me sound like a very application specific thing. A simple library around Promises could easily provide the same functionality, which is why React Async doesn't do any caching, you can simply bring your own.

So what would closer integration with Suspense look like? I'm hesitant to adopt the same API, and relying on interop between the two (i.e. using Async with Timeout or Placeholder) seems awkward and error prone. What are your thoughts on this?

Copy link

Choose a reason for hiding this comment

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

Personally I'm surprised that they even took the whole thing as far as providing a cache mechanism.

I haven't dug into code that does the suspending in React reconciler, but the mechanism how WIP react-cache package gets render to suspend is through throwing a pending promise -https://github.com/facebook/react/blob/master/packages/react-cache/src/ReactCache.js#L158 Before throwing the pending promise out to React renderer, one needs to store the promise somewhere, to access it again when promise gets resolved and render is retried, hence the need for react-cache. Also I think the react-cache won't be mandatory for suspense, so anyone can roll their own:

It also serves as a reference for more advanced caching implementations.
https://github.com/facebook/react/tree/master/packages/react-cache

Atleast that's how I understand it.

@ghengeveld ghengeveld merged commit e65e080 into master Dec 30, 2018
@ghengeveld ghengeveld deleted the useAsync branch December 30, 2018 21:44
@ghengeveld
Copy link
Member Author

@all-contributors please add @sibelius for review

@allcontributors
Copy link
Contributor

@ghengeveld

I've put up a pull request to add @sibelius! 🎉

@ghengeveld
Copy link
Member Author

@all-contributors please add @jimthedev for review

@allcontributors
Copy link
Contributor

@ghengeveld

I've put up a pull request to add @jimthedev! 🎉

@ghengeveld
Copy link
Member Author

@all-contributors please add @msokk for review

@allcontributors
Copy link
Contributor

@ghengeveld

I've put up a pull request to add @msokk! 🎉

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.

4 participants