jorgebucaran / hyperapp

1kB-ish JavaScript framework for building hypertext applications
MIT License
19.09k stars 780 forks source link

Rework Actions #162

Closed FlorianWendelborn closed 7 years ago

FlorianWendelborn commented 7 years ago

Current Behavior

Recently effects and reducers got merged into actions. Now the way to set the state is by returning an object.

Figure 1a

const someAction = (model, data, actions) => {
    return {
        state: 'change'
    }
}

The issue with that is that it makes the return functionality unavailable for (former) effects, which means that it is no longer possible to have actions return some temporary value. This is an important piece of functionality for complex apps.

In addition to that it makes the following patterns harder:

Figure 2a

const actions = {
    example: (model, data, actions) => {
        // since `actions.fetch()` depends on `model.url` the user is forced to add a reducer for this
        // simple example, since `return` would exit the function
        actions.setUrl('https://github.com')
        actions.fetch()
    },
    setUrl: (model, data) => ({
        url: data
    }),
    fetch: model => {/*...*/}
}

Figure 3a

const actions = {
    example: (model, data, actions) => {
        setTimeout(() => {
            // won't work, also needs an additional reducer
            return {
                state: "change"
            }
        })
    }
}

Proposed Behavior

Suppose there'd be a default action. Let's say setState or set that could be called with a model change. Above patterns would become:

Figure 1b

const actions = {
    example: (model, data, {set}) => set({
        state: 'change'
    })
}

Figure 2b

const actions = {
    example: (model, data, actions) => {
        actions.set({
            url: 'https://github.com'
        })
        actions.fetch()
    },
    fetch: model => {/*...*/}
}

Figure 3b

const actions = {
    example: (model, data, {set}) => {
        setTimeout(() => set({
            state: 'change'
        })
    }
}

Advantages

This has the advantage that any state change is always initiated by a actions.set() call. No more "sometimes return" and "sometimes add a reducer".

In addition to that it frees return, meaning that you can now do this:

const actions = {
    three: (model, data, {set}) => model.one + model.two,
    print: (model, data, actions) => console.log(actions.three())
}

which is amazing for more complex apps.


Meta

Original issue: What about setState?

jorgebucaran commented 7 years ago

No more "sometimes return" and "sometimes add a reducer".

What does "add a reducer" mean?

FlorianWendelborn commented 7 years ago

@jbucaran See setUrl in Figure 2a.

jorgebucaran commented 7 years ago

You are mixing your app's actions, with functionality that can (and should) be extracted to a separate module.

model.one + model.two

is better as

sum(a,b)=>a+b

That's some of what I mean by function composition from #159.

FlorianWendelborn commented 7 years ago

@jbucaran Believe it or no, I do in fact understand what functions are. If I'd share a real-world example it'd be bigger than the whole text of the issue. I chose the most minimalistic example that I could think of to show what's possible. This does in no way imply that I'd suggest using this action anywhere.

jorgebucaran commented 7 years ago

@dodekeract 🤔 I think the real issue here is that you dislike having an action (reducer) that updates only one bit of the model, like actions.setURL or actions.bumpCounter.

For such trivial things you'd prefer set({ whatever }) instead.

FlorianWendelborn commented 7 years ago

@jbucaran The real issue here is that merging effects into reducers removed most of the functionality of return. In addition to that it means that there is only one way to update the model - by calling set().

jorgebucaran commented 7 years ago

@dodekeract We're having a setState VS Redux/TEA discussion here.

The "best practice" is to have an action for every bit of the model your application can or wants to update.

That way your views only know what actions they need to call, but not how the action works.

Offering a set action out of the box goes against the pattern I'm promoting with hyperapp.

FlorianWendelborn commented 7 years ago

@jbucaran Hyperapp already isn't similar to redux anymore. It allows side-effects in actions.

jorgebucaran commented 7 years ago

since actions.fetch() depends on model.url the user is forced to add a reducer for this simple example, since return would exit the function

Depends, but if fetch really depends on model.url, then yes, you should update the URL (for example on reading input from a text box) and then call actions.fetch.

jorgebucaran commented 7 years ago

So, in short: you dislike creating actions for every piece of the model you have to update and you'd prefer a single set action out of the box, basically:

actions: {
   set: (_, data) => data
}

That's not my favorite way of doing it, but you are certainly free to use and abuse it :)

FlorianWendelborn commented 7 years ago

@jbucaran Except that it only solves a trivial, cherry-picked part of the problem. The other half is that return always implies a state update.

I'm not suggesting this because I can't implement this myself (in fact I included that implementation in my original issue to make it clear that this isn't my main point). I'm suggesting it because changing return behavior would require me to fork hyperapp to change it. The issue I'm facing is that I have a really complex WebRTC app that has to do a lot of stuff and in order to enable code composability I have to split up actions into multiple ones.

Said action-splitting is incredibly hard without being able to use return. Using non-hyperapp functions also makes no sense in that case, since the signature of the functions I need to call is model, data, actions - which coincidentally is the same as hyperapp's actions have.

jorgebucaran commented 7 years ago

@dodekeract Said action-splitting is incredibly hard without being able to use return. ...action splitting

Can you show a more concrete example? Maybe I can help you refactor.

zaceno commented 7 years ago

My take on what happened here (not saying it's wrong -- just saying... what I see) is that before effects and reducers were merged, reducers were equivalent to 'Msg' types in TEA. Effects were mostly like Cmd:s perhaps (async ops, can send msg:s).

Merging the two together to actions is a significant departure from TEA, in that now we have one thing that can sometimes act like a 'Msg' (when you return an object), and sometimes like a Cmd (when you return a promise or return nothing).

So while this merge of effects+reducers->actions doesn't exactly reduce the functionality of hyperapp, it does represent a move away from TEA and so you can't exactly reproduce the same patterns you would in TEA.

If hyperapp is going to keep with this pattern, I feel like some changes are needed to restore the elegance lost. 'Overloading' the return statement with hidden meanings (different things happen depending on what you return) looks to me like an inelegance that should be remedied somehow. This solution may not be it... but something?

jorgebucaran commented 7 years ago

I disagree with regards to any elegance being lost. On the contrary, I think the current model is simpler and just as elegant, if not more elegant. TEA is confusing and difficult at first. It used to be way worse when they were using StartApp, though.

jorgebucaran commented 7 years ago

'Overloading' the return statement with hidden meanings (different things happen depending on what you return) looks to me like an inelegance that should be remedied somehow.

I think it's pretty simple actually. If I was pushed to compare the current architecture to the original reducers/effects architecture, I would say: if you return something, then it's like the old reducers. If you don't return anything, then it's like the old effects.

In reality, however, here's how I see the current architecture compared to the original one: we didn't merge reducers with effects, we simply removed effects and renamed what was left (reducers) to actions, a more broader and reasonable term that boils down to functions.

I don't see this as hyperapp going out of its way either. Before, a reducer that returned undefined, i.e., didn't return anything, would cause the view to be rendered and also try to merge undefined with the current model, none of which makes any sense.

Think about it, first, actions behave like the old reducers, we merge their return value with the model. We certainly skip any view renders if your ~reducer~ action returns nothing, as merge(model, undefined) would be useless, but even if we didn't you could still use them as effects (they would cause a useless view render however).

The only special case is, admittedly, promises, but then everything is "special" about promises in JavaScript land.

jorgebucaran commented 7 years ago

One thing, before you could create a reducer like this:

reducers: {
   refresh: () => {}
}

to forcefully render the view, now that is:

actions: {
   refresh: _ => _
}

I don't know if such action is really useful though.

rbiggs commented 7 years ago

Hmmm... I'm all exited about using async/await in a project to kick the tires. But I'm wondering if there'll be a problem with them in the new version of actions. Internally async/await uses promises.

zaceno commented 7 years ago

@jbucaran

we didn't merge reducers with effects, we simply removed effects and renamed what was left (reducers) to actions

Ok that explanation clicks with me :) I now think I better understand how you see it.

... but still: reducers didn't use to be able to call other reducers. In Elm, Msg:s are something more specific than "just functions" -- each one represents a singular way that state can be transformed. Losing this sort of 'pure' reducer still irks me a little.

Anyway, I don't mean to derail this issue with my nostalgia for the "good old days" (2 weeks ago? ;) )

I just mean to say I (personally) feel like something was lost ("elegance", "purity"... not sure what to call it), and that I interpret @dodekeract's suggestion as an attempt to bring back some of what was lost, within the frame of this new architecture.

jorgebucaran commented 7 years ago

@rbiggs No problem, the docs have an example with async await.

@zaceno still: reducers didn't use to be able to call other reducers.

Correct. My take is that JavaScript is not Elm. Also, Elm is not the panacea. If Elm doesn't do it, I won't do it, doesn't work for me.

I feel this is still TEA under the hood, but simpler.

zaceno commented 7 years ago

@jbucaran

I feel this is still TEA under the hood, but simpler

This is true. I may feel like we're further away now, but still clearly born of ~the~ TEA

rbiggs commented 7 years ago

The TEA? Tiny Encryption Algorithm? 🤔

jorgebucaran commented 7 years ago

The Elm Architecture.

jorgebucaran commented 7 years ago

@dodekeract I don't think we'll be changing the API back to reducers/effects, but I have an idea that does not break the API and may help a bit.

if (result == null || typeof result.then === "function") {
-  return result
} else {
  for (var i = 0; i < hooks.onUpdate.length; i++) {
    hooks.onUpdate[i](model, result, data)
  }

  model = merge(model, result)
  render(model, view)
}
+ return result
jorgebucaran commented 7 years ago

This is just a little tweak to what's essentially the same as what we had before, but the action's result is always returned.

FlorianWendelborn commented 7 years ago

@jbucaran Wouldn't this mess with the model?

jorgebucaran commented 7 years ago

@dodekeract This would let you create a hack in which the caller of an action can get another action's return value. Totally not recommended, but better than nothing.

jorgebucaran commented 7 years ago

In terms of the API, nothing would break and it would make actions rather more "consistent", since wrapped actions would now return what the original action returns.

jorgebucaran commented 7 years ago

One thing though, this would make it impossible for actions/reducers to return the actions object in a future version. But I don't know if that's something I want anyway and it would be a sort of special behavior that would need to be documented, whereas my current suggestion is what someone that just saw hyperapp for the first time would expect, maybe.

jorgebucaran commented 7 years ago

@dodekeract Going to close here as there is nothing actionable, but as always, feel free to continue the discussion.

FlorianWendelborn commented 7 years ago

nothing actionable

Sure...