Skip to content

Add useSteppingAff hook #72

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

Merged
merged 1 commit into from
May 30, 2023
Merged

Conversation

jship
Copy link
Contributor

@jship jship commented May 24, 2023

This PR adds a useSteppingAff variant of the useAff hook that preserves the previous async effect's result up until the next async effect's run completes. We use this variant at Lumi in cases where we'd like to avoid showing a page load indicator when refetching data from the backend, as the stepping behavior allows us to show the previous value right up until we've finished the API calls.

deps ->
Aff a ->
Hook (UseAff deps a) (Maybe a)
useAff' initUpdater deps aff =
Copy link
Member

@pete-murphy pete-murphy May 26, 2023

Choose a reason for hiding this comment

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

What do you think about exporting useAff' to allow a user to define custom hooks built on top of it? Contrived example, but I could imagine maybe wanting to pass something other than identity or const Nothing as the initUpdater if wanting to track loading state along with the Aff's result (to preserve and render the previous result but disable a button while some request is in flight, e.g.)

useReloadingAff
  :: forall deps a
   . Eq deps
  => deps
  -> Aff a
  -> Hook
       (UseAff deps { isLoading :: Boolean, result :: a })
       (Maybe { isLoading :: Boolean, result :: a })
useReloadingAff deps aff =
  React.useAff' initUpdater deps aff'
  where
  initUpdater = (map <<< map) (_ { isLoading = true })
  aff' = map { isLoading: false, result: _ } aff

Copy link
Member

Choose a reason for hiding this comment

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

I'd want to have a pretty convincing use case. It's easy enough to copy/paste for current edge cases, so I worry it's not worth exposing that complexity.

Copy link
Member

Choose a reason for hiding this comment

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

But happy to defer to others since I'm not an active user right now.

Same goes for merging/publishing this.

Copy link
Member

@pete-murphy pete-murphy May 27, 2023

Choose a reason for hiding this comment

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

It's easy enough to copy/paste for current edge cases, so I worry it's not worth exposing that complexity.

Keeping the external API simple makes sense to me 👍 In cases where I've wanted anything other than the Maybe a state that useAff returns I'd just use a separate useState and use useAff for scheduling. So for the snippet I had above I think I'd probably just do

useReloadingAff deps aff = React.do
  state /\ setState <- useState Nothing
  useAff deps do
    liftEffect $ setState (map (_ { isLoading = true }))
    result <- aff
    liftEffect $ setState \_ -> Just { isLoading: false, result }
  pure state

which is not much more complicated than the version using useAff'.

I'm not really an active user either aside from side projects. Lumi's use case for useSteppingAff seems like enough motivation for merging this as-is, but I'll leave this open for feedback for a couple more days before merging.

@pete-murphy pete-murphy merged commit 2a0b598 into purescript-react:main May 30, 2023
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.

3 participants