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 new prop to be able to skip the initial fetch on component mount #70

Open
wants to merge 5 commits into
base: next
Choose a base branch
from
Open

Add new prop to be able to skip the initial fetch on component mount #70

wants to merge 5 commits into from

Conversation

markusylisiurunen
Copy link

Description

Added a new prop for Async and useAsync to be able to skip the initial render when component mounts. This can be useful, for example, for debounced actions.

Breaking changes

Should not have any breaking changes.

Checklist

  • Implementation for both <Async> and useAsync()
  • Added / updated the unit tests
  • Added / updated the documentation
  • Updated the PropTypes
  • Updated the TypeScript type definitions

@codecov
Copy link

codecov bot commented Aug 4, 2019

Codecov Report

Merging #70 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   98.83%   98.85%   +0.01%     
==========================================
  Files          10       10              
  Lines         689      697       +8     
  Branches      161      168       +7     
==========================================
+ Hits          681      689       +8     
  Misses          6        6              
  Partials        2        2
Impacted Files Coverage Δ
packages/react-async/src/propTypes.js 100% <ø> (ø) ⬆️
packages/react-async/src/Async.js 100% <100%> (ø) ⬆️
packages/react-async/src/reducer.js 100% <100%> (ø) ⬆️
packages/react-async/src/useAsync.js 98.93% <100%> (+0.04%) ⬆️
packages/react-async/src/status.js 100% <100%> (ø) ⬆️

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 f22acd0...e84ba05. Read the comment docs.

@markusylisiurunen
Copy link
Author

@ghengeveld Have the deploy jobs been working before or have they just been failing?

@ghengeveld
Copy link
Member

This is interesting. Can't you use deferFn instead of promiseFn to achieve this behavior? I'm intrigued by the debounce use case, so this is certainly valuable. Not sure it warrants an additional option though.

@ghengeveld
Copy link
Member

ghengeveld commented Aug 5, 2019

@ghengeveld Have the deploy jobs been working before or have they just been failing?

You can ignore those, the deploys are in ci/circleci: examples now. I've disabled now and deploy/netlify.

It only really deploys from master by the way.

@wyqydsyq
Copy link

@ghengeveld from what I have seen so far it appears deferFn does not resolve the value of the underlying promise, meaning you can't persist data returned from it cleanly (without passing setData into the deferred function) #87

@ghengeveld ghengeveld changed the base branch from master to next August 26, 2019 11:05
@async-library async-library deleted a comment from vercel bot Aug 26, 2019
@sbogdaniuk
Copy link

any progress on it?

@ghengeveld
Copy link
Member

ghengeveld commented Dec 10, 2019

The work being done in https://github.com/async-library/future/tree/core will address the use case of debounced actions. However this is not going to be released any time soon.

As for this PR, I haven't heard back from @markusylisiurunen regarding the reason to use promiseFn with skipOnMount over deferFn. The behavior being toggled with the skipOnMount option added here is already available by using deferFn. I have no intention of adding new props for use cases that can already be implemented by other means. So unless there's a solid use case for skipOnMount over deferFn this PR is going nowhere.

@markusylisiurunen
Copy link
Author

@ghengeveld Yeah, you are right. The behaviour can be implemented by using the deferFn, you just need to keep track of when the first render is happening and when it's an update render and call the deferFn accordingly. The reason for this was basically just to make this process a bit easier and cleaner. I understand that adding new props just for a specific use case may not be the best thing to do, so feel free to close this PR.

@sbogdaniuk
Copy link

Let's say we have search, and user should enter at least 3 characters.

const doSearch = async (query) => { ... }

const [search, setSearch] = useState('')

// trigger with useEffect
const { run, cancel, ... } = useAsync({
  deferFn: doSearch,
})

useEffect(() => {
  if (search.length > 2) {
    run(search)
  } else {
    cancel()
  }
}, [search])


// triggered with skip change
const { ... } = useAsync({
  promiseFn: doSearch,
  skip: search.length < 3
})

And you prefer to trigger it using via useEffect, over skip?

@markusylisiurunen
Copy link
Author

@sbogdaniuk That was basically the reason I was thinking about adding a flag for skipping with the promiseFn. It is possible to do with useEffect and deferFn but, at least in my opinion, it can be quite verbose. Maybe I was trying to solve a problem that is not really a problem but rather my hate towards multiple random useEffect blocks..

@ghengeveld
Copy link
Member

ghengeveld commented Dec 10, 2019

@sbogdaniuk You could probably use watchFn for that:

const doSearch = async (query) => { ... }
const [search, setSearch] = useState('')

const watchFn = ({ search }, prev) => search.length > 2 && search !== prev.search
const { ... } = useAsync({ deferFn: doSearch, watchFn, search })

Edit: oh wait that's only for promiseFn, obviously, so it would still start a request on first render. So yeah, useEffect is the way to go here.

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