jorgebucaran / hyperapp

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

What about setState? #159

Closed FlorianWendelborn closed 7 years ago

FlorianWendelborn commented 7 years ago

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

What about exposing a actions.setState function which can be called from everywhere where actions are available instead? This way an effect could still return whatever it wants when called by a different action (I actually used that a lot) and code like this becomes easier to do:

function example () {
    actions.room.set(room) // reducer
    actions.room.socket.join() // effect
    // the key here is that socket.join depends on model.room
}
function example () {
    actions.setState({room})
    actions.room.socket.join()
}

This would also fix use-cases where big actions need to be split into multiple actions, but need to return data that is not a promise. Hard to describe, but I actually also used that for WebRTC SDP patching. I worked around this, but overall I think that this might make hyperapp easier to use for more advanced apps.

And yes, I'm aware that setState() would basically be implemented as setState: (_, data) => data.

jorgebucaran commented 7 years ago

@dodekeract And yes, I'm aware that setState() would basically be implemented as setState: (_, data) => data.

Then, why not just add it to your actions?

FlorianWendelborn commented 7 years ago

Because I think it would get rid of the inconsistencies between reducers and effects. It was your idea to only have one type of action.

jorgebucaran commented 7 years ago

@dodekeract the key here is that socket.join depends on model.room

What do you mean and what problem are you having specifically?

FlorianWendelborn commented 7 years ago

@jbucaran exactly what I said. socket.join depends on model.room. So I can't return and then run another action since that's not how return works.

jorgebucaran commented 7 years ago

@dodekeract What is example? Sorry, if you don't try harder, don't expect further replies.

FlorianWendelborn commented 7 years ago

@jbucaran It's an action.

jorgebucaran commented 7 years ago

And what's the problem again?

FlorianWendelborn commented 7 years ago

@jbucaran That model.room is required for socket.join to work. If I'd just use a return {{room}} there it wouldn't work since return cancels the function. That's why the example code needs a reducer or a setState method.

In addition to that it would allow actions to return whatever they want to their caller, since return isn't used by hyperapp anymore.

jorgebucaran commented 7 years ago

Sorry, the example is not clear to me. If you can create a simple CodePen/fiddle demonstrating what the problem is, I'd definitely take a look though.

FlorianWendelborn commented 7 years ago

@jbucaran http://codepen.io/dodekeract/pen/zZzRPX?editors=0010

EDIT: Added more comments to better explain the things this proposal would change.

zaceno commented 7 years ago

I like this proposal!

I too have found myself making a generic reducer-action setState: (_, data) => data, just to be able to update the model at multiple points within an effect. It's "boilerplate:y" enough that it might as well be built in.

FlorianWendelborn commented 7 years ago

@zaceno I'm also proposing that return does what it's supposed to do - return a value to the caller. What's your opinion about that? (see codepen for details on why I think this is good)

jorgebucaran commented 7 years ago

@dodekeract I get it now. Thanks for making the pen, and save everyone time! :)

So, you want to use an action to return some intermediary value which will be used by another action, etc. I think the problem is that you want to use an hyperapp action for something that is not, but just a regular function, usually a third party helper that you can import in your module or declare on the same main file.

You could use a Promise, but that would make things more complicated and you don't need that.

I think you have two great options, (1) communicate only via the model, so update it with some intermediary data, similar to the .setState proposal or (2) create a regular function that returns what you want.

I think the best approach is using a function that does what you need like we've been doing since the dawn of functionkind.

All you need is (love) and more function composition.

EDIT: Like in this example. It would be weird to use an action for humanizeTime, reducer, effect, or whatever one may want to call it, because it is not an action, but just a helper function.

FlorianWendelborn commented 7 years ago

@jbucaran The functions I have in my use-cases are actions though. And it makes no sense to use the model for that. The content of function results don't belong into the model. They never get rendered, they aren't important in the long-run (more than 10ms) and they might even overwrite each other. They depend on both the model and other actions. This is one of the reasons why I didn't like the effects/reducers mixup - it breaks a lot of functionality for the sake of looking more minimalistic.

jorgebucaran commented 7 years ago

And it makes no sense to use the model for that. ... The content of function results don't belong into the model.

Yes, I totally agree, I wouldn't use the model for that either. I think what works is creating one or more function that can help modularize your action.

jorgebucaran commented 7 years ago

This is one of the reasons why I didn't like the effects/reducers mixup - it breaks a lot of functionality for the sake of looking more minimalistic.

I think it simplifies the architecture and pushes the user to rely less on "architecture" to get the job done.

jorgebucaran commented 7 years ago

I too have found myself making a generic reducer-action setState: (_, data) => data, just to be able to update the model at multiple points within an effect. It's "boilerplate:y" enough that it might as well be built in.

If you really need to update the model, that's okay, but I think @dodekeract does not want to update the model for intermediary values he needs inside an action. The real issue is he wants to keep all the functionality as actions, correct?

I think that's the issue. My answer is that not all your app functionality is necessarily a hyperapp action.

I'm closing here because I don't think we should update the API for this, but if you have another example or use case you think I'm missing, please post it.

jorgebucaran commented 7 years ago

@dodekeract A non-hyperapp function would be a possible workaround, but you'd have to pass both model and actions which seems to be quite weird if it's not a hyperapp action.

That's not a workaround, that's one way (if not the way) to go about this.

EDIT: If you want to create a different issue to talk about the real point: how to return something from actions which are not async, go ahead 👍.

FlorianWendelborn commented 7 years ago

@jbucaran A function that takes model, actions and data. Hmm - where do I know this concept from? Oh, right - an action.

jorgebucaran commented 7 years ago

@dodekeract You can still try to come up with a more real example that shows why the approach I proposed is/would be a disadvantage! 😊

FlorianWendelborn commented 7 years ago

@jbucaran I can't share the source. It's proprietary.

jorgebucaran commented 7 years ago

@dodekeract Sure. Create one you can then. Things don't get done just by asking, you need to demonstrate the problem.

zaceno commented 7 years ago

@dodekeract

I'm also proposing that return does what it's supposed to do - return a value to the caller. What's your opinion about that? (see codepen for details on why I think this is good)

Well, I'm certainly no expert on the design philosophies in hyperapp, but fwiw I think it makes sense.

When the decision was made to combine effects and reducers, we ended up in this situation where actions are supposed to be either a reducer (which should return a new state, hence it's name) or an effect which should be able to return anything. It seems like an odd place to be in now. This proposal would make things elegant again. (Again, from my non-expert perspective)

So yes, I believe I agree with this aspect too, of your proposal. Sure it means lots of people will have to rewrite their actions, but hyperapp is still in it's infancy and this is exactly the time to get it right, and not worry too much about backwards-compatibility.

jorgebucaran commented 7 years ago

@zaceno Sorry, but we are not separating actions into reducers and effects again, unless it is demonstrated why that would be better than having only actions.

FlorianWendelborn commented 7 years ago

@jbucaran This proposal never separated actions into effects and reducers. I actually see it like the proper way to merge both perfectly and without loss of functionality.

It would get rid of the whole concept of reducer. Previously we only got rid of the word, which was one of my reasons for disagreeing on merging effects and reducers into actions.

jorgebucaran commented 7 years ago

@dodekeract If you need to have a setState action, then why can't you just add it yourself? I don't really see what's the point in a framework providing such a minimal boilerplate.

Also, what about people who don't need it? I don't, but I would be forced to have it since it would be a new default.

FlorianWendelborn commented 7 years ago

@jbucaran The point is to free return so that it can do what it's supposed to do and to make async patterns easy for other users too.

jorgebucaran commented 7 years ago

@dodekeract What async patterns are currently not easy? I think they are extremely simple now.

Examples please.

FlorianWendelborn commented 7 years ago

@jbucaran Look at my example. It needs a reducer that only exists because it's unable to use return since that'd quit the function. If that would be streamlined it's easier to pick up for beginners.

Right now an async action can't return a new state update:

app(
    actions: [
        (model, data, actions) => {
            setTimeout(() => {
                // won't work
                return {n: "ewState"}
            })
        }
    ]
)

It is forced to use a reducer. Having setState or whateverYouWantToCallIt would make this just the same as the usual model update.

jorgebucaran commented 7 years ago

@dodekeract That has nothing to do with "async patterns".

jorgebucaran commented 7 years ago

@dodekeract Can you create a new issue?

Add new actions.setState default action

Why
How
Example

etc.

FlorianWendelborn commented 7 years ago

@jbucaran The proposal also includes changing the return behavior back to how it used to work.

Actually, now that I think about it your approach was to merge effects into reducers (reducer behavior stays the same) and my approach is to merge reducers into effects (effect behavior stays the same). 🙂

jorgebucaran commented 7 years ago

@dodekeract Ah, that makes things more clear. The reason you'd need setState is because we'd be removing reducers.

lukejacksonn commented 7 years ago

@dodekeract how hard would it be to create a PR for this? If the diff is not huge, which it shouldn't be right? Then usefulness vs cost can be better assessed. For what it is worth I can see some value in it, not sure if this is the ideal solution but I think trying to determine what actions return and how to chain them is a valuable discussion to have.

FlorianWendelborn commented 7 years ago

@jbucaran Well. Not "really" removing, but make them optional.

I'd still consider (model, data, {setState}) => setState({/*...*/}) a reducer.

FlorianWendelborn commented 7 years ago

@lukejacksonn It's trivial to create a PR, excluding tests that is. Quite sure that there'd be a lot of changes in those.

jorgebucaran commented 7 years ago

Yeah, now I finally understand what we are talking about.

jorgebucaran commented 7 years ago

This is more opaque. For example, instead of:

app({
  model: 0,
  actions: {
    add: model => model + 1,
    sub: model => model - 1
  },
  view: (model, actions) =>
    <div>
      <h1>{model}</h1>
      <button onclick={actions.add}>Increment</button>
      <button onclick={actions.sub}>Decrement</button>
    </div>
})

You'd need to do it like this:

app({
  model: 0,
  view: (model, actions) =>
    <div>
      <h1>{model}</h1>
      <button onclick={_ => actions.setState(model + 1)}>Increment</button>
      <button onclick={_ => actions.setState(model - 1)}>Decrement</button>
    </div>
})

Which is less code, but it's also worse code, because you have now coupled the view with the implementation of the action, which among other abominable things, makes it more difficult to create reusable components, and mocks the entire model+view+actions architecture.

Sure, you could also do this:

app({
  model: 0,
  actions: {
    add: (model, _, actions) => actions.setState(model + 1),
    add: (model, _, actions) => actions.setState(model - 1)
  },
  view: (model, actions) =>
    <div>
      <h1>{model}</h1>
      <button onclick={actions.add}>Increment</button>
      <button onclick={actions.sub}>Decrement</button>
    </div>
})

But now that's more code, more boilerplate and not as elegant as the current solution.

FlorianWendelborn commented 7 years ago

@jbucaran Yes, I see the boilerplate issue. Part of it gets solved by using set instead of setState (we don't use the word state anyway, only chose it since react used that) and maybe changing how actions are called.

jorgebucaran commented 7 years ago

@dodekeract I think we can all agree that the name is not the real issue here. If we are offering TEA out of the box (and we are), incorporating this default set action would be a major sin in the Elm doctrine.

FlorianWendelborn commented 7 years ago

@jbucaran I think that's just about semantics. The only thing that changes is that return is now set().

jorgebucaran commented 7 years ago

@dodekeract I think that's just about semantics. The only thing that changes is that return is now set().

No, it's not. It changes the architecture. Like I said above, I don't want to mix the implementation of an action with the view code. That's basically how React works without Redux. This point is very important!

The ultimate goal of any action is to update the model, one way or another.

Now, if only that would be good enough. If a lot of your code is doing async stuff, how do you use actions to do async stuff? Return a Promise! Which makes sense.

Now, if only that would be good enough. Not everyone can return a Promise. So, we let you return nothing which is familiar to callback hell adventurers.

I thought this was enough, but it seems you've found an "edge" case (correct me if I'm wrong). Which is, you want sync actions that return some value which may be part of some work you may be doing inside another action.

My solution would be to simply use functions.

FlorianWendelborn commented 7 years ago

@jbucaran

I don't want to mix the implementation of an action with the view code

That only happens if users decide to do it. If I'd implement setState myself it'd be possible with the current codebase as well. Nothing unique to my suggestion.

jorgebucaran commented 7 years ago

@dodekeract Right now an async action can't return a new state update:

  app(
    actions: [
      (model, data, actions) => {
        setTimeout(() => {
          // won't work
          return { n: "ewState" }
        })
      }
    ]
  )

It is forced to use a reducer. Having setState or whateverYouWantToCallIt would make this just the same as the usual model update.

It's pretty evident this won't work the way you expect given our current, and still primitive, understanding of the laws of physics.

Inside setTimeout, you usually want to do call another action that updates the bit of the model that you care about.

jorgebucaran commented 7 years ago

If I'd implement setState myself it'd be possible with the current codebase as well. Nothing unique to my suggestion.

Yes, but you'd have to implement it first, out of the box, HyperApp won't let you shoot yourself in the foot 😄.

FlorianWendelborn commented 7 years ago

@jbucaran The point of that example is that there are currently two ways to update the model. Either use return or add a reducer. return won't always work.

jorgebucaran commented 7 years ago

@dodekeract AFIK return always work. If you need to update the model, just return something!

actions: {
  love: model => ❤️
}

It works ✨.

FlorianWendelborn commented 7 years ago

@jbucaran You just quoted the example where it wouldn't work. I explained my reasoning behind it and my claim that return won't always work.

jorgebucaran commented 7 years ago

Which is obvious, because you are inside a callback. Sorry, are you suggesting callbacks in JavaScript is confusing or something like that?

actions: {
  writeToDb: (model, data, actions) => {
    myDB.write(data, ok => ok ? actions.happyReducer() : actions.sadReducer())
  }
}
FlorianWendelborn commented 7 years ago

@jbucaran Of course it's obvious. I always try to pick obvious examples. Still doesn't change the fact that there are currently two ways to update the model.

  1. return
  2. reducer => return