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

What about setState? #159

Closed
FlorianWendelborn opened this issue Mar 12, 2017 · 51 comments
Closed

What about setState? #159

FlorianWendelborn opened this issue Mar 12, 2017 · 51 comments

Comments

@FlorianWendelborn
Copy link

FlorianWendelborn commented Mar 12, 2017

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
Copy link
Owner

jorgebucaran commented Mar 13, 2017

@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
Copy link
Author

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
Copy link
Owner

jorgebucaran commented Mar 13, 2017

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

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

@FlorianWendelborn
Copy link
Author

@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
Copy link
Owner

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

@FlorianWendelborn
Copy link
Author

@jbucaran It's an action.

@jorgebucaran
Copy link
Owner

And what's the problem again?

@FlorianWendelborn
Copy link
Author

FlorianWendelborn commented Mar 13, 2017

@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
Copy link
Owner

jorgebucaran commented Mar 13, 2017

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
Copy link
Author

FlorianWendelborn commented Mar 13, 2017

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

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

@zaceno
Copy link
Contributor

zaceno commented Mar 13, 2017

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
Copy link
Author

@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
Copy link
Owner

jorgebucaran commented Mar 14, 2017

@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
Copy link
Author

FlorianWendelborn commented Mar 14, 2017

@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
Copy link
Owner

jorgebucaran commented Mar 14, 2017

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
Copy link
Owner

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
Copy link
Owner

jorgebucaran commented Mar 14, 2017

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
Copy link
Owner

jorgebucaran commented Mar 14, 2017

@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
Copy link
Author

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

@jorgebucaran
Copy link
Owner

jorgebucaran commented Mar 14, 2017

@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
Copy link
Author

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

@jorgebucaran
Copy link
Owner

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

@zaceno
Copy link
Contributor

zaceno commented Mar 14, 2017

@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
Copy link
Owner

@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
Copy link
Author

FlorianWendelborn commented Mar 15, 2017

@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
Copy link
Owner

jorgebucaran commented Mar 15, 2017

@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
Copy link
Author

FlorianWendelborn commented Mar 15, 2017

@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
Copy link
Owner

jorgebucaran commented Mar 15, 2017

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

Examples please.

@FlorianWendelborn
Copy link
Author

FlorianWendelborn commented Mar 15, 2017

@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
Copy link
Owner

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

@jorgebucaran
Copy link
Owner

jorgebucaran commented Mar 15, 2017

@dodekeract Can you create a new issue?

Add new actions.setState default action

Why
How
Example

etc.

@FlorianWendelborn
Copy link
Author

FlorianWendelborn commented Mar 15, 2017

@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
Copy link
Owner

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

@lukejacksonn
Copy link
Contributor

@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
Copy link
Author

FlorianWendelborn commented Mar 15, 2017

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

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

@FlorianWendelborn
Copy link
Author

@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
Copy link
Owner

jorgebucaran commented Mar 15, 2017

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

@jorgebucaran
Copy link
Owner

jorgebucaran commented Mar 15, 2017

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
Copy link
Author

@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
Copy link
Owner

@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
Copy link
Author

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

@jorgebucaran
Copy link
Owner

jorgebucaran commented Mar 15, 2017

@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
Copy link
Author

@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
Copy link
Owner

jorgebucaran commented Mar 15, 2017

@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
Copy link
Owner

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
Copy link
Author

@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
Copy link
Owner

jorgebucaran commented Mar 15, 2017

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

actions: {
  love: model => ❤️
}

It works ✨.

@FlorianWendelborn
Copy link
Author

FlorianWendelborn commented Mar 15, 2017

@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
Copy link
Owner

jorgebucaran commented Mar 15, 2017

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
Copy link
Author

@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

@jorgebucaran
Copy link
Owner

jorgebucaran commented Mar 15, 2017

Sorry, I don't understand. There is only one way to update the model.

actions: {
   updateTheModel: _ => newModel
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants