Closed sergey-shpak closed 5 years ago
Inexperienced developer here who likes hyperapp for its simplicity. A new API that doesnt appear to be simpler is troubling.
What the goals are for V2?
@Spankyed That question deserves a great answer. In the meantime, see the original discussion https://github.com/jorgebucaran/hyperapp/issues/672.
Beware, the issue is now outdated. Only some of the ideas described in it made it to the actual V2 goal list, but not all of them. V2 API changes are summarized in this comment and implemented in this PR.
Looks alright to me!
I do like getting away from objects for representing effects and their props.
Being able to return multiple effects from an action seems sensible, and would help composing actions where the different actions being composed all return effects.
I donโt see any point in being able to dispatch an action as if it were an effect. In fact I think Jorge might even want to prevent that.
@zaceno You'll be able to return multiple effects from an action as follows.
const actionReturningManyFx = state => [state, [MakeFx1(...), MakeFx2(...)]]
@sergey-shpak @zaceno I do like getting away from objects for representing effects and their props.
Effects need to be represented as objects.
We've discussed this a few times, but there's no specific issue dedicated to explaining why effects need to be represented with an object and not a function.
If you use a function to represent an effect, then it's impossible to test effects using a strict equality check. See this reply from the V2 Effects API thread.
@sergey-shpak No difference between action or effect call...
Actions and effects are conceptually different. Actions are the heart of Hyperapp. Need to update the state? Want to subscribe to mouse clicks? Want to subscribe to mouse moves? Want to produce side effects? Use actions.
Actions are our exchange currency; they're at the bottom of Hyperapp's economy.
Subscriptions, Effects and DOM events (might as well call them DOM subscriptions) are our trading goods and services.
If you need to use multiple actions in response to a button click, you can compose them. They all return raw state which you can merge to create more state. If you need an action to produce multiple side effects, e.g., writing to a database, sending some data to a server, and logging somethingโall at the same timeโyou can do it. Actions can return multiple effects. This is called batching effects and it will be built-into Hyperapp's core effects resolver algorithm.
const myActionDoesTooManyThings = state => [
state,
[
fxThatWritesToMyDB,
fxThatSendsDataToMyServer,
fxThatLogsDataSomewhere
]
]
Similarly, you may want to batch subscriptions. That too, will be built-into Hyperapp's subscription reconciler.
We want to be able to batch effects and subscriptions in Hyperapp and we will be able to do it. The current implementation in the V2 branch already has subscription batching, but effect batching is not implemented (yet).
@jorgebucaran
effects need to be represented with an object and not a function.
@sergey-shpak is not suggesting they be represented as functions, but that this:
{effect: myEffectFn, propA: 'foo', propB: 'bar'}
could instead be:
[myEffectFn, {propA: 'foo', propB: 'bar'}]
( unless I totally misunderstood)
That should be equally pure, safe for subscriptions, and easy for testing. Right?
@zaceno Sorry, I'm going to need more handholding as I have no idea what is being discussed here.
@jorgebucaran If I understand the suggestion correctly, @sergey-shpak is basically arguing that the dispatcher should work like this:
function dispatch (action, data) {
//call the action/effect
var retVal
if (Array.isArray(action)) {
const [fn, props] = action
retVal = fn(state, props, data)
} else {
retVal = action(state, data)
}
if (typeof retVal === 'function') {
//when a function is returned it means we started
//a subscription. Return the stop-function to the
//caller:
return retVal
} else if (Array.isArray(retVal) {
const [newState, ...effects] = retVal
//replace the global state, and maybe
//schedule a render update (if different)
updateState(newState)
//dispatch the effects
effects.forEach(f => dispatch(f, dispatch)
} else {
//it's just the new state:
updateState(retVal)
}
}
The main up-side to this (imo) is that the API is simpler and cleaner, because we no longer have to use specially formatted objects with reserved keys (i e {effect: f, ...props}
) to indicate an effect. Actions and effects both are represented as a [fn, props]
tuple.
I think this would make Hyperapp way more easy to explain and understand. The fact that you can't have a prop named effect
for an effect, while not exactly a problem in practice, is an ugly wart. When I see stuff like that in other apis/frameworks, I get the feeling that there's some over-engineering/over-complication going on.
The down-side to it is that the signature for effect-functions becomes: (state, props, dispatch) => {...}
. I don't think effect functions really should have access to the state. IMO the upside outweighs the downside though.
@sergey-shpak lists a couple other benefits which imo are less significant:
You can return multiple effects from an action. This is a great thing, but as @jorgebucaran pointed out, we'll have that anyway.
You can dispatch an action directly from another action. This is not really a benefit in my opinion, since it's not something anyone should do. On the other hand, there's no reason for Hyperapp to go out of it's way to prevent it either, because again, no one should be doing it anyway.
@zaceno What would be the user-facing changes?
You said "Actions and effects both are represented as a [fn, props] tuple", but currently actions can be either a function or a tuple.
@jorgebucaran, The main user facing change would be in the API for authoring effects. It will make for significantly better docs explaining how effects work and how to author them. It will give the impression of a more symmetrical, simple and thought-through whole (in my current and highly subjective opinion)
Thank you, @zaceno. That helps a bit. So, the suggestion is to use arrays to represent effects and subscriptions?
...and what do you mean by this comment?
...when a function is returned it means we started a subscription. Return the stop-function to the caller.
You said "Actions and effects both are represented as a [fn, props] tuple", but currently actions can be either a function or a tuple.
Yeah and that won't change.
It does however also means that an un-parameterized effect could also be represented by just a function. Which is bad if it's an inline defined function of course.
@jorgebucaran the comment about subscriptions is just that: because we're using dispatch
to start subscriptions (right?) then the subscription-update function needs to find out the stop functions. Which are returned from the subscription's "effect" function.
@zaceno Okay, I'll start tinkering with the idea of representing effects (and subscriptions) as arrays instead of { effect, ...props } and come back later with the result.
I have the bad habit of iterating through multiple implementations while working on an idea and finally when selecting my best option, leave no documentation of why the other alternatives didn't make it. I have a nagging feeling that I already experimented with this approach but had to abandon it for some reason. I hope I am wrong!
That should be equally pure, safe for subscriptions, and easy for testing. Right?
@zaceno yes, you took it absolutly right! )
@jorgebucaran, I understand why objects are used to describe effects (testing/subs diffing/debug/etc), but we can achieve this more consistently with other parts of HA (like actions and tuples)
Beside mentioned benefits, also there are additional features:
const action1 = () => {}
const actionN = [{}, effect({}) ]
// this wouldn't work as expected // const action = action1(actionN())
// but now you can const action = [ action1, actionN // effectN ]
- now it seems possible to call effect from an Event (should double-check)
```javascript
onClick={ effect, { action: update } }
One limitation is that effect shouldn't return cancel
function by default,
(because it will be called as an action and cancel effect). Possible solution
is to pass additional param when dispatching subscriptions to get cancellable function,
like cancel: dispatch(sub, true)
and then
const effect = (_, dispatch, props, isSubscription){
// ...
return isSubscription && cancel
}
@sergey-shpak One limitation is that effect shouldn't return cancel function by default.
What do you mean by default?
This is the implementation of the mouse moves subscription:
const effect = (props, dispatch) => {
const eventListener = event => dispatch(props.action, event)
addEventListener("mousemove", eventListener)
return () => removeEventListener("mousemove", eventListener)
}
export const moves = props => ({
effect: effect,
action: props.action
})
...how would it change?
@jorgebucaran , problem comes from recursion at dispatch
function, anything that returned by action (tuple or function) is dispatched until it gets final state object, so in our case if we write effect as
const moves = (_, dispatch, props){
const eventListener = event => dispatch(props.action, event)
addEventListener("mousemove", eventListener)
return () => removeEventListener("mousemove", eventListener)
}
and then use it like
const action = state => [
{ listeningMoves: true },
[ moves, { action: someAction } ]
]
dispatching [ moves, { action: someAction } ]
returns cancel
function, and due to recursion of dispatch it will be also dispatched, as result effect cancelled.
To properly behave we should pass some flag to effect to let it know that it is used as subscription and can be cancelable, so it should return cancel.
const moves = (_, dispatch, props, isSubscription){
const eventListener = event => dispatch(props.action, event)
addEventListener("mousemove", eventListener)
if(isSubscription) return () => removeEventListener("mousemove", eventListener)
}
and when starting/resolving subscriptions (somewhere inside of refresh
function)
{ cancel: dispatch(sub, true) } // true, passed as `isSubscription`
as well, dispatch
function should return effect cancel
function
// just an example of possible dispatch changes
var dispatch = function(obj, data) {
if (obj == null) {
} else if (typeof obj === "function") {
dispatch(obj(state, dispatch, data))
} else if (isArray(obj)) {
if (typeof obj[0] === "function"){
var cb = obj[0](state, dispatch, obj[1], data)
dispatch(cb);
return cb; // <-- returning effect cancel function
} else {
for ( var i = 0; i < obj.length; i++ )
dispatch(obj[i])
}
} else {
setState(obj)
}
}
@sergey-shpak Thanks. I have a simple question. Is the following:
const moves = (_, dispatch, props, isSubscription){
const eventListener = event => dispatch(props.action, event)
addEventListener("mousemove", eventListener)
if(isSubscription) return () => removeEventListener("mousemove", eventListener)
}
...the new way / suggested way to implement a subscription?
@jorgebucaran yes, the difference between subscriptions and effects is cancellable or not (at least from my point of view), so 'subscription is cancellable effect'
*I believe proper dispatch
changes will follow as soon as we discuss this approach in general
This doesn't look very appealing, but I'll tinker with it a bit as said in https://github.com/jorgebucaran/hyperapp/issues/765#issuecomment-426569664. ๐
My suggested dispatch
here https://github.com/jorgebucaran/hyperapp/issues/765#issuecomment-426558613 takes a slightly different approach to @sergey-shpak I realise.
My approach sidesteps the subscription issue, so subscriptions can be defined almost as they used to (except they get state
as the first argument to the effect-function).
but my approach doesn't allow you to compose actions as @sergey-shpak showed above, e g:
const multiAction = state => [state, action1, action2, action3]
...well you can but it won't work :P
I see where Sergey is coming from, but I still don't believe that dispatching multiple actions directly from another action is a valid/important use case.
[Edit: On the other hand, It's a bit unexpected and hard to explain why =>[state, someAction]
shouldn't work also. So my approach is not optimal either, I'll admit]
So while I like the idea of representing effects and subscriptions as tuples of [func, props]
, I think we should go with the version where subscriptions are still allowed to return a stop-function.
Actually... It's too bad we don't have test-cases for actions, effects and subs in the V2 branch yet. It would help to play around with variations on the api.
On the other hand, It's a bit unexpected and hard to explain why =>[state, someAction]
Why? I never expected that to work. But then again, I designed this part of the API looking at Elm.
This doesn't look very appealing.
100%, it would be great to get the clear effects and subs without any extra conditions inside
Maybe this issue should be reformulated to discuss the problem and not the implementation. I am quite satisfied with the current API, so I am having a hard time seeing the problem here.
Can you show me the problem with the current API? I am always open to changing my mind.
@jorgebucaran
From my perspective, the problem (definitely not huge problem, but still an ugliness/"smell") is that implementing an effect looks like this:
const MyEffect = (() => {
const effect = (props, dispatch) => {/* implementation */}
return props => ({effect, ...props})
})()
...which makes effect
a reserved property for no good reason. props
must be an object. It can't be a simple value.
Also we use the same api for subscriptions which is confusing (Am I writing an effect or a subscription?).
Contrast that with:
const MyEffect = (() => {
const effect = (props, dispatch) => {/* implementation */}
return props => [effect, props]
})()
Not only can props' keys be named anything (as one expects), but props
itself could just be a single value if one prefers. And it's completely symmetrical with how we define parameterised actions: [myAction, {some: 'prop'}]
or [myAction, 42]
I find that much more elegant & symmetrical.
Moreover, when effects and parameterised actions have the same shape, it seems there's potential to simplify dispatch
I've been experimenting with the following dispatch implementation with success so far.
var handleActionResult = function(actionResult) {
if (isArray(actionResult)) {
actionResult[1][0](
actionResult[1][1],
dispatch,
setState(actionResult[0])
)
} else {
dispatch(actionResult)
}
}
var dispatch = function(obj, data) {
if (obj == null) {
} else if (typeof obj === "function") {
handleActionResult(obj(state, data))
} else if (isArray(obj)) {
handleActionResult(obj[0](state, obj[1], data))
} else {
setState(obj)
}
}
The subscriptions reconciler also needs some changes. It's just a bit more code overall, but the result is the same API as described in #750 and #752 with @sergey-shpak simplification, without the named props restrictions.
const TimeDelay = (() => {
const effect = (props, dispatch) => {
setTimeout(() => dispatch(props.action), props.duration)
}
return props => [
effect,
{
action: props.action,
duration: props.duration
}
]
})()
const TimeTick = (() => {
const subscription = function(props, dispatch) {
const id = setInterval(
() => dispatch(props.action, { time: Date.now() }),
props.interval
)
return () => clearInterval(id)
}
return props => [
subscription,
{
action: props.action,
interval: props.interval
}
]
})()
cc @zaceno @sergey-shpak
I appreciate that we're considering different implementations and it doesn't look like this changes the overall architecture of 2.x, which is good. ๐
I'm also a personal fan of the tuple-all-the-things approach! ๐
With that said, how much longer do we want to leave the 2.x API open for changes? I'd like to keep the momentum going and move on to documenting/testing the new core along with implementing FX and subs to use with it.
@okwolf Agreed. I like @sergey-shpak's proposal so let me tinker with it a bit more (I want to make sure it's good). I still haven't completed the implementation, but it looks like I'll make it out alive. ๐ช
I'll try to wrap up these changes and push to the V2 branch ASAP.
Seems very promising. I like that we are drying up the signatures :pray:
@jorgebucaran checked your last dispatch function, it doesn't support multiple action compositions and returning state in array (const action = state => [{ updated: true }, effect]
)
My personal choice is this dispatch
implementation
Anyway, great work, specially on backward compatibility with current API effects and subs usage :+1:
@jorgebucaran
It's just a bit more code overall
That's surprising! I would have thought the symmetry between effects and parameterised actions would have allowed for some simplifications and code-reductions.
If I want to compose two actions all I need to do is call myAction(anotherAction())
. Of course, this doesn't work for actions that return effects.
What does that mean?
@zaceno dispatch
is simpler, but changes in the subscription reconciler will make up for most of it. But maybe I can do better. We'll see.
@jorgebucaran
Returning state in array What does that mean?
I mean following state update doesn't work with the latest dispatch implementation
const action = (state, props) => [
{ ...state. ...props },
effect({ action: someAction })
]
because action returns array and dispatch will try to call first element as a function, which is object
@sergey-shpak Can't reproduce. It definitely works here using the dispatch function I shared above.
const SergeyShpak = (state, props) => [
{ ...state. ...props },
effect({ action: someAction })
]
<button onClick={SergeyShpak}>Do Something</button>
SergeyShpak
returns an array, which we handle in handleAction
here:
var handleActionResult = function(actionResult) {
if (isArray(actionResult)) {
actionResult[1][0](
actionResult[1][1],
dispatch,
setState(actionResult[0])
)
Please check again! ๐๐
@jorgebucaran , Reproduced with following:
const effect = (props, dispatch) => dispatch(props.action);
const actionB = state => state
const actionA = (state, props) => [
{ ...state, ...props },
[ effect, { action: actionB } ]
];
app({
init: actionA({ init: true }),
view: state => h('div', {}, 'test'),
subscriptions: console.log,
container: document.body
})
@sergey-shpak Just a small typo.
app({
+ init: () => actionA({ init: true }),
- init: actionA({ init: true }),
view: state => h('div', {}, 'test'),
subscriptions: console.log,
container: document.body
})
@sergey-shpak Here you have a few examples that work as expected.
init
to stateapp({
init: { name: "Foo" },
view: state => <h1>{state.name}</h1>,
subscriptions: console.log,
container: document.body
})
init
to state from action resultconst simpleAction = name => ({ name })
app({
init: simpleAction("Bam"),
view: state => <h1>{state.name}</h1>,
subscriptions: console.log,
container: document.body
})
init
to state from result of parameterized actionconst simpleActionWithProps = (state, name) => ({ ...state, name })
app({
init: [simpleActionWithProps, "Bar"],
view: state => <h1>{state.name}</h1>,
subscriptions: console.log,
container: document.body
})
init
to action that takes some state and produces effectconst TimeDelay = (fx => props => [fx, props])((props, dispatch) => {
setTimeout(() => dispatch(props.action), props.duration)
})
const simpleActionWithProps = (state, name) => ({ ...state, name })
const actionWithDelay = state => [
{ name: state },
TimeDelay({ action: [simpleActionWithProps, "Baz"], duration: 1000 })
]
app({
init: () => actionWithDelay("Fum"),
view: state => <h1>{state.name}</h1>,
subscriptions: console.log,
container: document.body
})
init
to parameterized action that produces effectconst TimeDelay = (fx => props => [fx, props])((props, dispatch) => {
setTimeout(() => dispatch(props.action), props.duration)
})
const simpleActionWithProps = (state, name) => ({ ...state, name })
const actionWithDelay = (_, name) => [
{ name },
TimeDelay({ action: [simpleActionWithProps, "Toto"], duration: 1000 })
]
app({
init: [actionWithDelay, "Pow"],
view: state => <h1>{state.name}</h1>,
subscriptions: console.log,
container: document.body
})
@sergey-shpak The purpose of dispatch
is to dispatch an action, not effects. To produce an effect, we dispatch an action that returns an effect. That's it.
@jorgebucaran thank you! Looks great! :100:
@jorgebucaran https://github.com/jorgebucaran/hyperapp/issues/765#issuecomment-427569108 Looks like you are getting better at story telling ๐
This looks great btw ๐
Since tuples are a common signature the learning curve is short, HAV2 should have the same appeal as V1 ๐
@jorgebucaran is this planned to be included in the V2 branch soon or is it not settled yet?
Planned. All tuple is the new hot.
Done! ๐
Hyperapp V2 brings powerful concepts such as actions, effects and subs. (additional thanks to @jorgebucaran and the community), but the implementation doesn't look straightforward to me (I mean 'effects as objects', and so on)
After some research I came to one more possible approach (looks much simplier and generic to me)
So, what if?
Small changes have to be done to
dispatch
function and subscriptions resolving, but before creating PR and moving forward, it would be great to hear your thoughts*moreover,
subscriptions
now look like anaction
, and possibly can dispatch initial state, so init action can be omitted, like