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

Parameterization of actions in V2 #751

Closed
jorgebucaran opened this issue Aug 28, 2018 · 22 comments
Closed

Parameterization of actions in V2 #751

jorgebucaran opened this issue Aug 28, 2018 · 22 comments

Comments

@jorgebucaran
Copy link
Owner

jorgebucaran commented Aug 28, 2018

The goal of this issue is documenting and formalizing how to use parameterized actions. If you are not familiar with the new V2 Action API, please see #749.

In V2, the runtime (Hyperapp) calls our actions, not us. So, how do we pass a value to an action?

const IncrementBy = (state, { value }) => ({ value: state.value + value })

There have been two approaches on the table since the original V2 announcement was made in #672. Using a two-element array (henceforth referred to as tuple) or using an object.

Your mission, should you choose to accept it, consists of choosing the best between these two options. Good luck!

Options

Tuple Syntax

[Action, props]

Pros

  • Concise
  • Props need not be an object (a single primitive value works)
  • Can be used with effects and subscriptions seamlessly
  • Closer to the tuple syntax in elm

Cons

  • Using arrays as tuples may be odd to newcomers
  • We are going to use tuples to describe actions that produce side-effects too [newState, nextEffect]

Object Syntax

{
  action: Action,
  ...props
}

Pros

  • Descriptive
  • More idiomatic (people are used to objects)

Cons

  • Verbose
  • Awkward when used with effects and subscriptions
  • Reserves the name action (you can't have a prop named action in your state)

Examples

One-liner

Tuple-syntax
<button onClick={[IncrementBy, { value: 100 }]}>Add One Hundred</button>
Object-syntax
<button onClick={{ action: IncrementBy, value: 100 }}>Add One Hundred</button>

Multi-line

Tuple-syntax
<button
  onClick={[
    FollowUser,
    {
      userId: props.userId,
      follow: true
    }
  ]}
>
  Follow
</button>
Object-syntax
<button
  onClick={{
    action: FollowUser,
    userId: props.userId,
    follow: true
  }}
>
  Follow
</button>

Nested objects

Tuple-syntax
<button
  onClick={[
    EngageDrive,
    {
      warp: 10,
      course: {
        id: 113,
        mark: 7
      },
      impulse: false
    }
  ]}
>
  Engage
</button>
Object-syntax
<button
  onClick={{
    action: EngageDrive,
    warp: 10,
    course: {
      id: 113,
      mark: 7
    },
    impulse: false
  }}
>
  Engage
</button>

Todo item component

Tuple-syntax
import { h, app } from "hyperapp"
import { Check, Edit, UpdateTodo, Delete } from "./actions"

const HandleKeyDown = (state, data, { keyCode }) =>
  keyCode === 13 ? Edit(state, data) : state

export const TodoItem = ({ todo }) => (
  <li
    class={{
      completed: todo.completed,
      editing: todo.editing
    }}
  >
    <div class="view">
      <input
        type="checkbox"
        checked={todo.completed}
        onClick={[Check, { id: todo.id, isCompleted: todo.completed }]}
      />
      <label
        text={todo.description}
        onDoubleClick={[Edit, { id: todo.is, isEditing: true }]}
      />
      <button class="destroy" onClick={[Delete, { id: todo.id }]} />
      <input
        class="edit"
        id={`todo-${todo.id}`}
        type="text"
        value={todo.description}
        placeholder="Remember to..."
        onInput={[UpdateTodo, , { id: todo.id }]}
        onBlur={[Edit, { id: todo.id, isEditing: false }]}
        onKeyDown={[HandleKeyDown, { id: todo.id, isEditing: false }]}
      />
    </div>
  </li>
)
Object-syntax
import { h, app } from "hyperapp"
import { Check, Edit, UpdateTodo, Delete } from "./actions"

const HandleKeyDown = (state, data, { keyCode }) =>
  keyCode === 13 ? Edit(state, data) : state

export const TodoItem = ({ todo }) => (
  <li
    class={{
      completed: todo.completed,
      editing: todo.editing
    }}
  >
    <div class="view">
      <input
        type="checkbox"
        checked={todo.completed}
        onClick={{ action: Check, id: todo.id, isCompleted: todo.completed }}
      />
      <label
        text={todo.description}
        onDoubleClick={{ action: Edit, id: todo.is, isEditing: true }}
      />
      <button class="destroy" onClick={{ action: Delete, id: todo.id }} />
      <input
        class="edit"
        id={`todo-${todo.id}`}
        type="text"
        value={todo.description}
        placeholder="Remember to..."
        onInput={{ action: UpdateTodo, id: todo.id }}
        onBlur={{ action: Edit, id: todo.id, isEditing: false }}
        onKeyDown={{ action: HandleKeyDown, id: todo.id, isEditing: false }}
      />
    </div>
  </li>
)

Abandoned Options

A third option consists of currying the actions. Unfortunately, it has several disadvantages: poor debugability, non-declarative, invalidation of strict equality checks when diffing props from events, effects and subscriptions.

@SkaterDad
Copy link
Contributor

SkaterDad commented Aug 29, 2018

I think I lean towards the tuple version since it approaches Elm syntax more, but don't care that much.

The object version is certainly more "normal" JS.

From the library code perspective, the current V2 code which handles the object-style actions is pretty clean & intuitive.

    } else if (typeof obj.action === "function") {
      dispatch(obj.action(state, obj, data))

The array syntax would be a little more complex and uglier. (unless you skip the isArray check)

} else if (Array.isArray(obj) && typeof obj[0] === "function") {
  dispatch(obj[0](state, obj[1], data)
}

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Aug 29, 2018

@SkaterDad Thanks for your feedback, Mike. The implementation doesn't bother me personally. I'm just interested in reaching a consensus here. 🙏

I think I lean towards the tuple version since it approaches Elm syntax more, but don't care that much.

Neither approach is too wildly different from the other, and that is what makes this decision hard. I don't take issue with the verbosity of the approach using objects, but I haven't been writing V2 apps for a long time either.

Now, there is an actual problem when dealing with parameterized actions in effects and subscriptions.

How do you pass a value to this action?

MyEffect({ props, action: MyAction })

This works, but it's quite unpleasant.

MyEffect({ props, action: { action: MyAction, value: 42 } })

To avoid the redundancy and mitigate the issue, we can use a different property name, but the result is not very satisfying either.

MyEffect({ props, onComplete: { action: MyAction, value: 42 } })

I could live with that, but it means more to remember — was it onComplete or onSuccess? It would be nicer if effects treated the action property consistently.

I think the tuples win in this case

MyEffect({ props, action: [MyAction, { value: 42 }] })

But is this a real use case? I never had to do something like this.

cc @zaceno @okwolf @Mytrill

@okwolf
Copy link
Contributor

okwolf commented Aug 29, 2018

If parameterizing actions passed to FX is a valid use case, then personally I prefer the 2-tuple syntax.

With that said, it's tough for me to see what that use case would be. Since the effect can already be parameterized, and FX have full control over the data they dispatch, when would you need to parameterize the actions passed at the time of FX creation? 🤔

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Aug 29, 2018

@okwolf It should be: when would you need to parameterize the actions dispatched at the time the effect is back with a result. I can't think of a scenario off the top of my head either.

Is this the only reason to like the tuple syntax?

@lukejacksonn
Copy link
Contributor

For me, the tuple is the clear winner. The idea of keeping the action function and its args separated (rather than all in the same object) is a pro that you forgot to mention imo.

My bad analogy being [red, blue] is more self explanatory than {...purple}

From initial response on here and in twitter. It doesn't seem like people are finding the tuple syntax too hard to digest and comprehend.

@zaceno
Copy link
Contributor

zaceno commented Aug 29, 2018

Like everyone so far (?) I also prefer the tuple-variant. It makes intuitive sense. I also like the idea of props being allowed to be values or objects, and I don't like reserving words if it can be avoided.

@tetradice
Copy link
Contributor

I agreed with the object syntax at the first point. But now I think that tuple syntax is better. Because object syntax has some problems as @jorgebucaran and others have mentioned.

It is true that newcomers are difficult to understand, but if the main problem is only that one, it is primarily the better to choose a tuple.

@frenzzy
Copy link
Contributor

frenzzy commented Aug 29, 2018

Please add your reaction to this comment

👍 if your vote for Tuple syntax [Action, props]
👎 if your vote for Object syntax { action: Action, ...props }

@maxholman
Copy link

maxholman commented Aug 29, 2018

Another vote for tuple syntax, would be good to have multiple argument support too (I believe it would be more intuitive for newbies, helps avoid having to name things, eases refactoring, eases typings etc etc)

@ddmng
Copy link

ddmng commented Aug 29, 2018

Tuple looks shorter and requires no reserved word, +1 for me

@SkaterDad
Copy link
Contributor

@maxholman would be good to have multiple argument support too

That's a great idea. It is annoying sometimes to be forced into an object just to pass 2 arguments.
Were you thinking it would look like this?

[TheAction, arg1, arg2, ...]

@okwolf
Copy link
Contributor

okwolf commented Aug 29, 2018

I'm against using positional parameters when there are multiple. With destructuring well-supported today, objects give named parameters, which I prefer.

@maxholman
Copy link

@SkaterDad Were you thinking it would look like this?
[TheAction, arg1, arg2, ...]

Yes, exactly. I've also got some idea of how to implement it elegantly in dispatch...I think.

@okwolf With destructuring well-supported today, objects give named parameters

Agreed, however, my suggestion is more to address the issues I mentioned: newbies, intuition (ie arguments being accepted just like a regular function does), refactoring, typings etc.

@ericdmoore
Copy link

The vote only includes {👍: "Array/Tuple", 👎:"Object"} but it does not include the {🤷‍♂️: " I'm fine with either/don't care/ I have not really thought deeply about this beyond the idea that one seems shorter than another, and I don't like typing"}

So keep in mind that somethings are not solved best via pure democracy. For example, even in America, you don't generally get to vote on if you think "Free Speech" is a good idea.

Frankly, my vote would be best represented by {vote: "👎🤷‍♂️" }

@ericdmoore
Copy link

My comment was not intended to be disparaging of any prior comments, but merely a reaction of recent things at work where a prior voting process led to a "short term win" - and now a "long term frustration". The lesson learned was that the direction of a vote, and the conviction of that vote are nice to have together.

@ddmng
Copy link

ddmng commented Aug 30, 2018

@ericdmoore I agree that there could be a {🤷‍♂️: " I'm fine with either... option: I would change mine to that if existed. Anyway I won't mind if @jorgebucaran choses to follow the least popular option.

@ericdmoore
Copy link

I like your style! You sir, @ddmng, are what we need more of in this world :)

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Aug 31, 2018

@ericdmoore The lesson learned was that the direction of a vote, and the conviction of that vote are nice to have together.

I'd like to learn from that lesson Eric. Anything else you can share with us?


I felt this decision deserved an issue for two reasons: (1) it is a key aspect of the API and (2) I want to document the decision making process. This issue is our official record showing how we arrived at this decision and I estimate will be very useful in the future.

@jorgebucaran jorgebucaran mentioned this issue Aug 31, 2018
14 tasks
@ericdmoore
Copy link

@jorgebucaran - At the risk of taking your statement too literally :)

I felt this decision deserved an issue for two reasons: (1) it is a key aspect of the API and (2) I want to document the decision making process. - jorgebucaran

Absolutely, and I think the discourse here is civil and productive, and the public rationale and record keeping are laudable too.

As far as lessons learned, There are two other things at play: "disagree and commit" and "the level of consideration"

Disagree and commit

Numerous well run companies benefit from a "disagree and commit" ethos. Where someone can strongly disagree, make their points, and then even if the final decision goes the against their point of view, they are able to muster up the strength to trudge through "the mud" that the may have even predicted. This is largely lacking in American politics, but some companies are still a strong hold of this very productive mentality. And I just wonder in the world of Yet Another JS framework - I wonder how much of a disagree and commit ethos you can really have. Minor disagreements, sure - but large ones?? Seems to me that strong disagreements in composition patterns results in forks, attempts to maintain old world forks, or leaving all together.

Consideration Given

What i alluded to in the comment above was that I have a preference, but it's based on a small amount of consideration.

Frankly, my vote would be best represented by {vote: "👎🤷‍♂️" }

Separate But Concrete Example
If you had put the assumptions of HAV2 to a vote - I would have said " meh- I'm fine with HAV1". But having thought about it a little more - I think V2 is pretty great. And the "having thought about it" is a result of seeing all the code sandboxes,jsFiddles, and codepens that people throw on Slack, as well as seeing other issues crop up here, etc.

If this was a "work setting" someone would likely spend a few days and attempt to simulate what a sufficiently complex system looks under both of the options. Where the goal is to let the voters have a more tangible, a more visceral, or a at least a more informed opinion on the ramifications of their opinion. So that each vote cast could be more enlightened than the one formed by minor considerations

Conclusion

This thread and countless others serve as great milestones of how a community engages the member, and I find most of the HA community members more thoughtful about programming patterns than my usual comrades in a commercial setting. Perhaps this type of thing is less needed with people able to mentally visualize these things.

In short "disagree and commit" generally allows for more insightful decision making. And you can get their by really appreciating the nuance of a decision (high consideration) or by assuming the "voice of the crowd" is seeing something I am not (low consideration)

Action Items

Seeking "High Consideration" - we could include more complex "Code Samples"
Fine with "Low Consideration" - count my {vote: "👎🤷‍♂️" } as still able to "disagree and commit"

(( Next week, intend to start working on a complex Code Sample assuming object destructuring && I will post it here as I am able to get to it. ))

Sorry for the pseudo philosophy rant - especially if your Anything else you can share with us? was sarcasm.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Sep 1, 2018

👋 @ericdmoore. Fascinating. I really enjoyed reading this. And no, my question wasn't sarcasm in the slightest. I'm sorry you doubted that for a second. It was pure curiosity. Thank you for taking the time to write this!

I think I could've added more than those 3 arbitrary examples, so I've added a new, more realistic example to the list above.

If you had put the assumptions of HAV2 to a vote - I would have said: " meh- I'm fine with HAV1".

I tried in #672. However I still have more to improve in my dealings with the community. For example, I use slack too much. I feel that's only acceptable if it's met with an equally active operation on GitHub through issues which leave a (more or less) permanent record and document the history of the project.

FWIW I've already started this by revamping the labels used in the project and retroactively searching old issues, summarizing their resolution and preemptively locking them in some instances.

Anyhow, I'm really glad to hear you are on board with V2. 🎉


@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Sep 1, 2018

@ericdmoore I've added a new, more realistic example to the list above.

My first impression is that the object syntax gets a bit annoying with the action keyword scattered all over the code, so I am now swaying in favor of the tuple syntax. I admit it's not as nice as Elm, but syntax-wise, the JavaScript function invocation is and has always been C-like anyway.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Sep 7, 2018

Tuples win. I'll update #749 to clarify the details.

TL;DR
<button onClick={[MyAction, { foo, bar, baz }]}>Do Something FooBarBazy</button>

Repository owner locked as resolved and limited conversation to collaborators Sep 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants