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

V2 What if? #765

Closed
sergey-shpak opened this issue Oct 3, 2018 · 45 comments
Closed

V2 What if? #765

sergey-shpak opened this issue Oct 3, 2018 · 45 comments

Comments

@sergey-shpak
Copy link
Contributor

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?

// Quite Simple Effects
const effect = (_, dispatch, props, sub) => {
  const id = setTimeout(() =>
    dispatch(props.action, { effect: true }), 1000);
  return sub && (() => clearTimeout(id));
};

// Quite Simple Actions
export const actionA = state => [
  { ...state, updateA: true },
  actionB // or [ actionB, { props: 'someBprop' } ]
]

export const actionB = state => [
  { ...state, updateB: true },
  // No difference between action or effect call
  [ actionC, { direct: true } ],
  [ effect, { action: actionC } ]
  // ... and more effect/action calls
]

export const actionC = (state, dispatch, data) => ({
  ...state, ...data, updateC: true
})

app({
  init: actionA({}),
  view: state => h('div', {}, 'test'),
  // quite simple subscriptions
  subscriptions: state => [
    [ effect, { action: actionA } ],
    [ actionB, { params: 'B' }]
  ],
  container: document.body
})

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 an action, and possibly can dispatch initial state, so init action can be omitted, like

app({
  view: state => h('div', {}, 'test'),
  subscriptions: state => [
    !state && { initial: true },
    [ effect, { action: actionA } ],
    [ actionB, { params: 'B' }]
  ],
  container: document.body
})
@spankyed
Copy link

spankyed commented Oct 3, 2018

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?

@jorgebucaran
Copy link
Owner

jorgebucaran commented Oct 3, 2018

@spankyed That question deserves a great answer. In the meantime, see the original discussion #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.

@zaceno
Copy link
Contributor

zaceno commented Oct 3, 2018

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.

@jorgebucaran
Copy link
Owner

@zaceno You'll be able to return multiple effects from an action as follows.

const actionReturningManyFx = state => [state, [MakeFx1(...), MakeFx2(...)]]

@jorgebucaran
Copy link
Owner

jorgebucaran commented Oct 3, 2018

@sergey-shpak @zaceno I do like getting away from objects for representing effects and their props.

tl;dr

Effects need to be represented as objects.

Why?

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.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Oct 3, 2018

@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.

tl;dr

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).

@zaceno
Copy link
Contributor

zaceno commented Oct 3, 2018

@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?

@jorgebucaran
Copy link
Owner

@zaceno Sorry, I'm going to need more handholding as I have no idea what is being discussed here.

@zaceno
Copy link
Contributor

zaceno commented Oct 3, 2018

@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.

@jorgebucaran
Copy link
Owner

@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.

@zaceno
Copy link
Contributor

zaceno commented Oct 3, 2018

@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)

@jorgebucaran
Copy link
Owner

jorgebucaran commented Oct 3, 2018

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.

@zaceno
Copy link
Contributor

zaceno commented Oct 3, 2018

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.

@zaceno
Copy link
Contributor

zaceno commented Oct 3, 2018

@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.

@jorgebucaran
Copy link
Owner

@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!

@sergey-shpak
Copy link
Contributor Author

sergey-shpak commented Oct 3, 2018

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:

  • it's possible to compose multiple actions:
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)
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 
}

@jorgebucaran
Copy link
Owner

@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?

@sergey-shpak
Copy link
Contributor Author

sergey-shpak commented Oct 3, 2018

@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)
    }
  }

@jorgebucaran
Copy link
Owner

@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?

@sergey-shpak
Copy link
Contributor Author

sergey-shpak commented Oct 3, 2018

@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

@jorgebucaran
Copy link
Owner

jorgebucaran commented Oct 3, 2018

This doesn't look very appealing, but I'll tinker with it a bit as said in #765 (comment). 👍

@zaceno
Copy link
Contributor

zaceno commented Oct 3, 2018

My suggested dispatch here #765 (comment) 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.

@zaceno
Copy link
Contributor

zaceno commented Oct 3, 2018

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.

@jorgebucaran
Copy link
Owner

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.

@sergey-shpak
Copy link
Contributor Author

This doesn't look very appealing.

100%, it would be great to get the clear effects and subs without any extra conditions inside

@jorgebucaran
Copy link
Owner

jorgebucaran commented Oct 3, 2018

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.

@zaceno
Copy link
Contributor

zaceno commented Oct 3, 2018

@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

@jorgebucaran jorgebucaran mentioned this issue Oct 4, 2018
@jorgebucaran
Copy link
Owner

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.

Effect Example
const TimeDelay = (() => {
  const effect = (props, dispatch) => {
    setTimeout(() => dispatch(props.action), props.duration)
  }
  return props => [
    effect,
    {
      action: props.action,
      duration: props.duration
    }
  ]
})()
Subscripttion Example
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

@okwolf
Copy link
Contributor

okwolf commented Oct 4, 2018

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.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Oct 4, 2018

@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.

@selfup
Copy link
Contributor

selfup commented Oct 4, 2018

Seems very promising. I like that we are drying up the signatures 🙏

@sergey-shpak
Copy link
Contributor Author

@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 👍

@zaceno
Copy link
Contributor

zaceno commented Oct 5, 2018

@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.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Oct 5, 2018

Multiple action composition

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.

Returning state in array

What does that mean?

@jorgebucaran
Copy link
Owner

@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.

@sergey-shpak
Copy link
Contributor Author

@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

@jorgebucaran
Copy link
Owner

jorgebucaran commented Oct 5, 2018

@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! 🙏😉

@sergey-shpak
Copy link
Contributor Author

sergey-shpak commented Oct 6, 2018

@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
})

@jorgebucaran
Copy link
Owner

@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
})

@jorgebucaran
Copy link
Owner

@sergey-shpak Here you have a few examples that work as expected.

set init to state

app({
  init: { name: "Foo" },
  view: state => <h1>{state.name}</h1>,
  subscriptions: console.log,
  container: document.body
})

set init to state from action result

const simpleAction = name => ({ name })

app({
  init: simpleAction("Bam"),
  view: state => <h1>{state.name}</h1>,
  subscriptions: console.log,
  container: document.body
})

set init to state from result of parameterized action

const simpleActionWithProps = (state, name) => ({ ...state, name })

app({
  init: [simpleActionWithProps, "Bar"],
  view: state => <h1>{state.name}</h1>,
  subscriptions: console.log,
  container: document.body
})

set init to action that takes some state and produces effect

const 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
})

set init to parameterized action that produces effect

const 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
})

@jorgebucaran
Copy link
Owner

@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.

@sergey-shpak
Copy link
Contributor Author

@jorgebucaran thank you! Looks great! 💯

@selfup
Copy link
Contributor

selfup commented Oct 6, 2018

@jorgebucaran #765 (comment) 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 🎉

@okwolf
Copy link
Contributor

okwolf commented Oct 7, 2018

@jorgebucaran is this planned to be included in the V2 branch soon or is it not settled yet?

@jorgebucaran
Copy link
Owner

jorgebucaran commented Oct 7, 2018

Planned. All tuple is the new hot.

Done! 🎉

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

6 participants