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

Rework Actions #162

Closed
FlorianWendelborn opened this issue Mar 15, 2017 · 30 comments
Closed

Rework Actions #162

FlorianWendelborn opened this issue Mar 15, 2017 · 30 comments

Comments

@FlorianWendelborn
Copy link

FlorianWendelborn commented Mar 15, 2017

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

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

What does "add a reducer" mean?

@FlorianWendelborn
Copy link
Author

FlorianWendelborn commented Mar 15, 2017

@jbucaran See setUrl in Figure 2a.

@jorgebucaran
Copy link
Owner

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

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

jorgebucaran commented Mar 15, 2017

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

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

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

FlorianWendelborn commented Mar 15, 2017

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

@jorgebucaran
Copy link
Owner

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

jorgebucaran commented Mar 15, 2017

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

FlorianWendelborn commented Mar 15, 2017

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

jorgebucaran commented Mar 15, 2017

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

zaceno commented Mar 15, 2017

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

jorgebucaran commented Mar 15, 2017

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

jorgebucaran commented Mar 15, 2017

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

jorgebucaran commented Mar 15, 2017

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

rbiggs commented Mar 15, 2017

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

zaceno commented Mar 15, 2017

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

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

zaceno commented Mar 15, 2017

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

rbiggs commented Mar 15, 2017

The TEA? Tiny Encryption Algorithm? 🤔

@jorgebucaran
Copy link
Owner

The Elm Architecture.

@jorgebucaran
Copy link
Owner

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

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

FlorianWendelborn commented Mar 21, 2017

@jbucaran Wouldn't this mess with the model?

@jorgebucaran
Copy link
Owner

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

jorgebucaran commented Mar 22, 2017

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

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

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

@FlorianWendelborn
Copy link
Author

FlorianWendelborn commented Apr 16, 2017

nothing actionable

Sure...

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