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

Update actions.md #1065

Closed
wants to merge 1 commit into from
Closed

Update actions.md #1065

wants to merge 1 commit into from

Conversation

Seanmcdon
Copy link

added a section for custom payloads that also include the event.
I used code from an issue solution buried in the repository.

added a section for custom payloads that also include the event. 
I used code from an issue solution buried in the repository.
@jorgebucaran
Copy link
Owner

Isn't this just a pattern, though? Maybe it doesn't belong to the documentation.

@icylace
Copy link
Contributor

icylace commented May 17, 2021

While this pattern looks handy it does feel like it belongs in userland.

Also, the PR mentions usage of event.stopPropagation() as a justification for the pattern it proposes but the code examples don't show where it would go or that it would need to be called from within an effect in order to maintain the purity of actions.

@Seanmcdon
Copy link
Author

Seanmcdon commented May 17, 2021

@icylace , you’re right I should have showed how to use the event object in the code.

However there is a use case for providing an argument to the action. Is this in the docs anywhere?

h(“div”, { onclick: withPayload(arg) } ...

//specifically, in this example ‘arg’ is:

(e) => [    MyAction,    { id: i, evt: e }  ]

Is there any other way to accomplish this specific payload? I’m not sure I could have figured out this logic on my own without finding @jorgebucaran ‘s code in one of the discussions, which I basically copied here.

In this use-case the argument is a function that returns a 2-tuple (a unique part of hyperapp actions). This is more that just currying, right?

const withPayload = (filter) => (_, x) => filter(x)

Something is going on beyond my current understanding (sorry this is my own ignorance 😆)

@zaceno
Copy link
Contributor

zaceno commented May 17, 2021

I agree with @Seanmcdon that this use case of creating a payload from combined event-data and static/state data is important and not immediately obvious how to do. It would be good to have a brief example in the docs. And it would make sense to have it in, or immediately following the "wrapped actions" section.

Some might recall there used to be a feature known as "payload filters" which was for precisely this use case. But we removed the feature because we realized it was redundant as it can be achieved through return an action-payload tuple (exactly as @Seanmcdon explains)

That said, I'm at odds with the example in this PR. I feel that introducing the withPayload function up front is redundant and obfuscates the actual technique – probably confusing readers. Also, while there is nothing wrong with it, it is not how I would do it. There is enough subjectivity here, and variety of use cases, that I don't think the docs should favor any particular approach.

I would rather have an example where the method for dispatching an action with a payload that is a combination of event and static data, was demonstrated inline in a view. Clearly, without any indirection.

Perhaps something like:

const InputField = (state, {name, value}) => ({
  ...state,
  fields: {
    ...state.fields,
    [name]: value
  }
})


//...

h("input", {
  type: "text",
  oninput: (state, event) => [
    InputField,
    { name: "hobbies", value: event.target.value }
  ]
}) 

I don't think we need to introduce any withPayload or similar function, as presumably readers are familiar enough with javascript to figure out how to reduce repetition with their own kind of helper functions appropriate for their use case. But if we do, then we should be clear it is only one possible way and that it does not rely on any hyperapp features.

@Seanmcdon
Copy link
Author

Seanmcdon commented May 18, 2021

Yes, I agree @zaceno. This is a straightforward example without any extra fluff. I will close this and re-think about how to place it in the docs.

@Seanmcdon Seanmcdon closed this May 18, 2021
@Seanmcdon Seanmcdon deleted the patch-1 branch May 18, 2021 02:37
@jorgebucaran jorgebucaran added the docs You read it here first label May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs You read it here first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants