-
Notifications
You must be signed in to change notification settings - Fork 780
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: Props not passed to effect function #748
Comments
@zaceno I've removed the ActionFunction
// or
{ action: Function, ...props } Effect{ effect: Function, action: Function, ...props } Subscription{ effect: Function, action: Function, ...props } |
@jorgebucaran any particular reason for dropping support of dispatching FX directly? Having to create an action to return |
@okwolf I'm still open to change it, but what do you mean by particular reason? It's a logical default. Going out of our way to support actions returning effects ought to have a particular reason. I don't believe it is more annoying than merging the current state with the next state to produce the new state. const Increment = state => ({ ...state, value: state.value + 1 }) const IncrementRandom = state => [state, Random(..)] |
Reminds me of a previous discussion we had about updating the state by merge (V1) vs replace (V2). |
What if the state is itself an array? Just curious. |
Currently, it can't be. |
Ok, so not a bug then. Good to know :) I'm curious about the (A guess before you answer: Could it be for composing actions somehow?) |
@zaceno That's how you pass props/data to the action. If you specify only a function, we'll pass only the event data. |
🤔 Ok, I see, I think -- but how does the action get its props? I mean, in this example: const Increment = amount => state => ({...state, value: state.value + amount})
...
<button onclick={Increment(2)}>+2</button>
<button onclick={Increment(5)}>+5</button> if the Increment action is defined like this instead:
what does the button-part look like? |
An action / no user data. const Increment = (state, event) => state + 1
//
onclick={Increment} An action and user data. const Increment = (state, { number }, event) => state + number
//
onclick={{ action: Increment, number: 10 }} |
@jorgebucaran Thanks! Think I got it. So it would be the same if you wanted to dispatch an action with data in an effect? |
@zaceno I don't understand what you are asking. Could you rephrase the question please? |
@jorgebucaran never mind. I thought about it a bit and my question doesn't make sense anyway 😅 So basically, these both work: const Increment = amount => (state, event) => ({...state, value: state.value + amount})
...
<button onclick={Increment(3)}>+3</button> OR const Increment = (state, props) => ({...state, value: state.value + props.amount})
...
<button onclick={{action: Increment, amount: 3}}>+3</button> But the latter is preferable because it makes it possible to test the component's action with an equality check. In the former example, a test would have to call the action and compare results (and mock side effects et c). Is that about right? |
@zaceno The former is more difficult to test, yes. Also, since it creates an anonymous function, Hyperapp has no way to know the name of the action, making it impossible to do things like time travel. We still don't have that tech, but we know we'll need the name of the action when we do. |
I'm coming into this one a bit biased, since this extra boilerplate is one of my pet peeves with Elm that I was hoping to avoid. And we are allowing for directly returning the next state insead of forcing the tuple/array, but not for the next effect. In practical terms thinking like a user of Hyperapp and TEA in general it seems that the use cases for an action both immediately modifying the state and producing an effect that later modifies the state are pretty rare. It's usually one or the other. This API isn't really optimized for that. In addition to being more annoying to write, it's a bit more hostile to testing (especially in the case of chaining FX together) since all of your FX will likely turn into actions so you can get the current state for including in the tuple and give a name to this for calling. So there won't really be testing of just the FX themselves unless you write even more verbose code where you have to come up with names for both the action and the effect in the tuple it returns. Then there's the hard-to-track-down bugs this leads to when users accidently do what I would consider to be the intuitive thing and try to dispatch FX. Depending on the shape of the effect, it will either call the |
@okwolf Thank you for your input, it has been noted. |
I say let's start with the necessary features to get going and add later if we need to. A few issues when returning an effect from an action:
|
What do you mean? Don't we have to come up with a name for both the action and the effect always? I'm not following this. |
Not following this either. What are you talking about? |
@jorgebucaran I think we'll have to agree to disagree on whether attaching FX directly to events is an anti-pattern and which is easier for testing. Could you please explain what the disadvantage is with Cheers 🍻 |
This is related to not directly attaching FX to events since you would need to define another named action function instead of inlining it if you wanted to achieve full test coverage. |
In |
It's just as a bad as inlining an action to an event or a subscription.
Okay, now I understand. Yes, of course, and that's a type error.
Inlining effects is an anti-pattern as described above. |
Ok, this one I understand. Although you do have all the values in the effect object that can already be used for debugging purposes. It wouldn't be difficult to include a
This one I don't really understand. Defining the |
Nothing. Defining the effect function outside of the object when implementing the effect will correct this problem. 👍 |
Then I think we may be writing past each other. My request was to support |
@okwolf I'll explain it better. First, let me define what I mean by inlining an effect. I mean directly attaching an effect to an event, effect or subscription. onClick={MyEffect}
MySub({ action: MyEffect })
MyEffect({ action: MyEffect }) None of the above are currently supported in V2 and my position is strongly against it. Note the problem is with inlining effects, not with allowing actions to return an effect. We could allow actions to do that, and then tell users to be careful. But by allowing actions to return an effect we are encouraging users to inline effects. I like to think of actions as V2's one and only currency. In V2 you can:
|
Ok now I think we're almost on the same page. I understand the disadvantages of inlining FX meaning like so: app({
init: {},
view: () =>
button(
{
onClick: {
thisIsWhatImeanBy: "inlinedFX",
effect(props, dispatch) {
setTimeout(() => {
dispatch(actionAlsoInlined => ({ andThats: "a bad idea" }));
}, 1000);
}
}
},
"click me!"
),
subscribe: console.log,
container: document.body
}); But I don't agree with the downsides of this style of inlining: const SeparatelyDefinedActionToCallFromEffect = (
thisIsClearlyNotInline,
shouldBeDataButInsteadItsTheEffect
) => ({
andThats: "a good idea",
shouldBeDataButInsteadItsTheEffect
});
const SeparatelyDefinedEffect = {
thisIsAlso: "not inlined",
action: SeparatelyDefinedActionToCallFromEffect,
effect(props, dispatch) {
const dataThatNeverGetsPassed =
"because this effect is treated like an action";
setTimeout(() => {
dispatch(props.action, dataThatNeverGetsPassed);
}, 1000);
}
};
app({
init: {},
view: () =>
button(
{
onClick: SeparatelyDefinedEffect
},
"click me!"
),
subscribe: console.log,
container: document.body
}); You can see this failing here. What I have to write instead is: const SeparatelyDefinedActionToCallFromEffect = (thisIsClearlyNotInline, dataWorks) => ({
andThats: "a good idea",
dataWorks
});
const SeparatelyDefinedEffect = {
thisIsAlso: "not inlined",
action: SeparatelyDefinedActionToCallFromEffect,
effect(props, dispatch) {
const dataThatDoesGetPassed = "because this effect is wrapped in an extra no-op action"
setTimeout(() => {
dispatch(props.action, dataThatDoesGetPassed);
}, 1000);
}
};
// the annoyance isn't just having to write this
// but also testing it when it really couldn't care less what the state is
const ExtraActionJustToCallTheEffect = state => [
state,
SeparatelyDefinedEffect
]
app({
init: {},
view: () =>
button(
{
onClick: ExtraActionJustToCallTheEffect
},
"click me!"
),
subscribe: console.log,
container: document.body
}); You can see this running here. Perhaps the reason we don't have a middleware API yet is because it would allow adding support for this to |
@okwolf The following should illustrate what will and will not be possible. Not okonClick={{ effect, action }}
onClick={MyEffect}
MySub({ action: MyEffect })
MyEffect({ action: MyEffect }) OkonClick={Action}
onClick={{ action: Action, ...props }}
onClick={ActionThatReturnsEffect}
MySub({ action: ActionOrActionThatReturnsEffect })
MyEffect({ action: ActionOrActionThatReturnsEffect }) |
That's not the reason, but yes, you are right that I don't want to (and probably won't) allow extending |
@okwolf You are conflating effects with actions, therefore incorrectly arriving at the conclusion that an extra action is needed to produce the effect, when in fact, there are no extra actions. It's just an action. |
I have yet to see a middleware proposal that doesn't allow for extending |
https://github.com/hyperapp/hyperapp/blob/ac1bb7d5811d6511213532c992d7f49b72706013/src/index.js#L570
This is a bug I think, it should probably read:
The text was updated successfully, but these errors were encountered: