Remove update/action code and re-expose React's setState functions

[maddie927]

We've been finding the update/action pattern to be too verbose for basic use cases. There was also a bug with type checking actions correctly (#71). This update keeps the current types and lifecycle code while returning state updates to the more traditional setState and setStateThen.

[justinwoo]

I've been using the actions a lot and while it's verbose sometimes, even just making a SetState (state -> state) has been okay for me. Surely there's a compromise somehow to expose both the Action/update version and a normal setState version

[friedbrice]

I find that working with the Action/Update idiom makes pages (for me, at least) much easier to follow, mostly because it treats the page as a state machine, with clearly-defined transitions. I wonder, though, if we can still achieve a similar code structure in its absence. I'm thinking something like:

data Action = Fight Move | Use Item | Switch PkMn | Run

send setState' action = case action of
    Fight move ->
        ...
        setState' (...)

    Use item ->
        ...
        setState' (...)

    ...

component = ... where
    render props state setState =
        ...
        button
            { title: "Choose Eevee"
            , onClick: send setState (Switch eevee)
            }

The problem with something like this is that it relies on social conventions rather than API.

[hdgarrood]

I think if we are going to remove actions, the README.md probably needs a minor update to reflect that?

I am not deeply familiar with react-basic but since:

• the API provided by React itself for updating component state is just setState, and
• if we agree that there exist use cases for which the setState API is more useful than the current react-basic actions API,

then presumably it makes sense for setState to be the default option (i.e. the one we provide in React.Basic), and then an action-based API could be built on top of that (possibly also in react-basic?). I don't know enough about the implementation to know how feasible this is, admittedly.

If we do want to make both available, I think making setState the default API is definitely preferable to emulating setState on top of the actions API with something like data Action state = SetState (state -> state) i.e. adding another layer on top of the actions API whose only purpose is to get back to where we started.

The problem with something like this is that it relies on social conventions rather than API.

Yes, I think an actions-based API is probably not particularly useful unless we can be sure that state changes only happen via send / update.

[maddie927]

the API provided by React itself for updating component state is just setState

This is one of the arguments for hooks as well, and it allows for both of these patterns (as well as user-created ones) on a per-component basis. I'd rather stick with the least-breaking change possible until we've explored that.

[paf31]

I find that the reducer approach makes code reviews quite a bit harder, and potentially complicates the logic, since I have to look in at least two places to understand the logic (at least, since the update function can itself trigger more actions).

As for the argument about structuring code, I don't think actions improve anything there since each action can just be replaced by a named function (since actions are not part of the component's API but just an implementation detail).

I also would prefer setState to be the default, since that lets us (or our users) build things like reducers or other layers on top if they want to.

[codedmart]

I find that the reducer approach makes code reviews quite a bit harder, and potentially complicates the logic, since I have to look in at least two places to understand the logic (at least, since the update function can itself trigger more actions).

As for the argument about structuring code, I don't think actions improve anything there since each action can just be replaced by a named function (since actions are not part of the component's API but just an implementation detail).

I also would prefer setState to be the default, since that lets us (or our users) build things like reducers or other layers on top if they want to.

I agree with all these statements.

[maddie927]

#71 has been fixed (v7.0.0), so that bit of my original comment is no longer relevant.

[paf31]

Looks good. Let's leave this open for a little while longer to test it out and collect feedback, and then we can plan to merge and release if it looks okay.

An example of the recommended style for existing reducer-based components might be nice too, if only in the readme.

[maddie927]

Sounds good, I have a branch in progress in Gumball we can review soon.

[maddie927]

I've converted some internal Lumi components to use this branch. So far nothing's been hard or even drastically different. I tried a few different approaches depending on what seemed to make the most sense for the component:

• for simple components where the action/update machinery was just noise I removed it entirely
• for components where some centralized structure still made sense and there weren't too many actions, I made each action a helper function:

Before:

    update self = case _ of
      ToggleOpen ->
        UpdateAndSideEffects
          self.state { isOpen = not self.state.isOpen }
          \{ props, state } -> when state.isOpen props.onOpen
      Close ->
        Update
          self.state { isOpen = false }

After

    toggleOpen self = do
      self.setStateThen
        (_ { isOpen = not self.state.isOpen })
        do
          props <- readProps self
          state <- readState self
          when state.isOpen props.onOpen

    close self = do
      self.setState _ { isOpen = false }

• for larger components with lots of actions where I was after the fastest/safest approach I renamed the update function to send and fixed up each case to call self.setState instead of returning Update, or self.setStateThen instead of UpdateAndSideEffects (or simply executing the side effect in the same block if it didn't need to be deferred):

Before:

update self = case _ of
  IsDragging isDragging ->
    if self.state.isDragging == isDragging
      then NoUpdate
      else Update self.state { isDragging = isDragging }

After:

send self = case _ of
  IsDragging isDragging -> do
    unless (self.state.isDragging == isDragging) do
      self.setState _ { isDragging = isDragging }

The rest of the changes were related to moving/removing some event helpers which never should have been in the React.Basic module or were mostly redundant:

Before:

onClick: capture_ self $ CancelPendingFile value

After:

capture_ $ send self $ CancelPendingFile value

[justinwoo]

Surely some of this code could live in a Utils module or something

[maddie927]

Which code? StateUpdate? I think that's the main difference. Maybe a StateUpdate -> Effect Unit function would be a good idea. Or (Self -> Action -> StateUpdate) -> Self -> Action -> Effect Unit, though that's really the same thing.

[maddie927]

@justinwoo What do you think of this?

[justinwoo]

Looks nice, I think this will work for me, thanks.