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 useStateConst where update is always const #24

Merged
merged 2 commits into from
Apr 29, 2020

Conversation

jvliwanag
Copy link
Contributor

A common use case of useState does not rely on the update function. Adding useStateConst to address this. Not sure if I chose the best name though.

@megamaddu
Copy link
Member

megamaddu commented Apr 28, 2020

Thanks for the PR!

I'm a little hesitant to merge this for two reasons, though:

  1. The update pattern should be encouraged everywhere possible.
  2. The set pattern is trivial to create from the update pattern.

With both of these considerations in mind, I'm not sure making the api busier is worth it.


As for why the update pattern should be encouraged, it's primarily to avoid race conditions. Even "atomic" values like numbers and strings can run into issues. For example, this code has a bug:

counter /\ setCounter <- useStateConst 0
let increment = setCounter (counter + 1)

It ties the effect of incrementing the counter to the way React chooses to dispatch renders. If increment is called multiple times before React can completely finish a render cycle, some of those calls will seem to be "dropped". This is because for every call to increment in that render cycle, counter will be bound to a single value, which essentially turns every call to increment into setCounter 1. React can also choose to abort renders before they are complete and continue them later. This means the state variables bound in a render function should only be used to render the UI. All async behavior should only modify state via update functions:

counter /\ setCounter <- useState 0
let increment = setState \c -> c + 1

If it helps, _ syntax can be very helpful both for primitives and record updates:

let increment = setState (_ + 1)
-- assuming the state record has other fields as well:
let openModal = setState _ { isModalOpen = true }

But be careful to avoid _ for record updates when the inner value actually depends on the current value!

-- notice, "toggle" instead of "open"
let toggleModal = setState \s -> { isModalOpen = not s.isModalOpen }

Sometimes constant values do make sense. I've found, however, that it's often more specific to the type of update than it is to the type of state. The counter example works well here too:

counter /\ setCounter <- useState 0
let increment = setState \c -> c + 1
let reset = setState \_ -> 0

Here, a reset never depends on the previous state but an increment does.

It's also very easy to get the behavior setStateConst would provide using setState (even though I still wouldn't recommend it):

counter /\ updateCounter <- setState 0
let setCounter c = updateCounter \_ -> c

-- or

counter /\ setCounter <- rmap (_ <<< const) <$> setState 0

By the way, I usually find things read better when low level hook usage is packaged away in component-specific hooks. It often removes the setState \s -> some logic clutter which can make your render functions harder to read. This also makes it easy to extract logic once a particular hook becomes useful in more than one component. For example, this counter code from earlier (expanded a little):

component "Counter" \props -> React.do

  counter /\ setCounter <- useState 0
  let increment = setState \c -> c + 1
  let reset = setState \_ -> 0

  pure $ ...render some things

Could instead be written like this:

component "Counter" \props -> React.do

  { counter, increment, reset } <- useCounter 0

  pure $ ...render some things

  where
  useCounter initialValue = React.do
    counter /\ setCounter <- useState initialValue
    let increment = setState \c -> c + 1
    let reset = setState \_ -> initialValue
    pure { counter, increment, reset }

@jvliwanag
Copy link
Contributor Author

Hi @spicydonuts thanks for reviewing. You're right that whenever an update function is needed, then useStateConst is not the right way to go. As an aside, the example from the official react hooks looks like it would have a similar issue -- https://reactjs.org/docs/hooks-state.html .

I'm not proposing the useStateConst for the use cases you mentioned above though (counter and record updates). But having since used hooks, the coding style now seem to favor fine grained states rather than a monolithic state for a component.

Consider a react component with a form with 10 fields. Using hooks, we'd normally write 10 separate useState calls. If the fields aren't anything fancy, they're most likely be const.

Another place that the const version of the useState will be easier is when passing the updater as an argument to another function. So something like capture_ (setEnabled (const true)) doesn't read as well as capture_ (setEnabled ).

Last note is on my codebase that use this awesome purescript-react-basic-hooks, I normally name the setter side as update. Meaning:

firstName /\ updateFirstName <- useState ""

And then usually create an alias:

let setFirstName = updateFirstName <<< const.

Normally though I don't ever use the updateFirstName.

@megamaddu
Copy link
Member

Hmm.. I see what you mean. That React docs example is unfortunate though. 😅

What do you think of calling it useState'? useStateConst does make sense when you consider the implementation, but from a type perspective (and the module api) it's not super clear. The prime variant to me reads as "a version of useState with a different type".

@jvliwanag
Copy link
Contributor Author

I definitely prefer useState'. Updated the pull req to reflect this.

@megamaddu megamaddu merged commit 4725768 into purescript-react:master Apr 29, 2020
@jvliwanag jvliwanag deleted the use-state-const branch April 29, 2020 03:10
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.

2 participants