Closed cassiozen closed 3 years ago
Work in progress branch: https://github.com/cassiozen/useStateMachine/tree/updatedSend
Relates to the "Old" proposal:
I think if you can make the assumption that updating the context before sending an event is almost always the preferred course of action then this API helps move toward that.
My question then would be, do you really need send
at all? Or could you just call update()(‘EVENT’)
in the simple case? Perhaps there are enough cases without context to make this tedious.
On the other hand, if it can’t be assumed that updating context should happen before a transition (although I can’t see why this would be the case) then personally I’d be against the change solely for aesthetic reasons.
I can’t shake the initial feeling that having two ways to send an event inside an effect would ultimately be confusing. Given the old cliche that we write code once and read it ten times, I think I’d lean in favour of writing two parentheses (update()('EVENT’)
) than seeing code that uses two ways to send an event. But this is just my gut reaction!
Relates to the "Old" proposal:
One other issue that this API doesn’t solve is providing an API that would allow avoiding unnecessary re-renders inside effects.
I haven’t looked at the code here but given calls to update
and send
may happen outside of a user interaction, React won’t batch the state updates. If you provide an interface that allows a user to pass both an event and an update inside the same function call you’d be able to batch those updates given they are available in the same scope.
Perhaps this isn’t important, but neither of the existing and proposed solution fixes this (apart from the alternative) to my understanding.
Relates to the "Old" proposal:
Another option is an options object, which gives you access to both without the horrors of TS overloads.
send({ event: string, updater: fn })
Curried APIs aren't particularly common in JS libraries. It makes sense to me to use them where they're necessary for inference, but I'd get some strange looks from people at work if I introduced this to them.
Relates to the "Old" proposal:
@RichieAHB:
My question then would be, do you really need send at all?
Agreed. It was there for backwards compatibility - but it does add confusion
I think if you can make the assumption that updating the context before sending an event is almost always the preferred course of action then this API helps move toward that.
Even though I do think this assumption is true, I'd rather provide an API that lets the user do both.
If you provide an interface that allows a user to pass both an event and an update inside the same function call you’d be able to batch those updates given they are available in the same scope.
Agreed. Batching was already on the backlog, but this would be a good opportunity to also tackle that.
All things considered, this is the new proposal (which will make @threehams happy):
effect(assign) {
assign({update: contextUpdaterFn, event: 'TOGGLE'});
}
Updated the proposal
I like it, but where would you put context to cover https://github.com/cassiozen/useStateMachine/issues/33?
I think the above makes sense. There are a few points that it’s seems worth calling out:
transition
is closer to the semantics than assign
- but that is personal preference!updater
function redundant, where a simple context
field would do:update({ context: getContext() + 1, event: ‘EVENT’ })`
Aside from this consideration I think it looks neat and solves a few other problems (and happy for this to be labelled “out of scope” but thought I’d raise anyway!)
I'm tending to not add a getContext to effect for the next version:
I can certainly be convinced otherwise, but for now my idea is to keep the updater function for the context.
Assuming that there is no requirement to change the signature in the next version then personally I think this is a good change!
I think the unification under assign
looks very good. Here's just a few things floating around in my head when I see it:
assign({update: { data } , event: "EVENT"})
or assign({update: (context) => fn(data,context) , event: "EVENT"})
, in addition to the examples in the OP. Maybe this is a good time to introduce that?assign
API looks roughly like good ol' this.setState
and I wonder if it's best to do assign(update, event)
rather than relying on special key names. Though effects with only event transitions: assign(undefined, 'something')
, assign({}, 'something')
, and assign(x=>x, 'something')
, might be annoying.assign
looks more like setContext
, emit
or publish
(like, publish this new context and move to this state)assign(update).transition(event)
?We'd like to simply return a partial update to the context, that is assign({update: { data } , event: "EVENT"})... Maybe this is a good time to introduce that?
That would be cool, still wanna do that, but I don't have the bandwidth to do it myself right now. I'll push for an update with event
which is more urgent and try this later.
With the above, the assign API looks roughly like good ol' this.setState and I wonder if it's best to do assign(update, event) rather than relying on special key names. Though effects with only event transitions might be annoying.
And we're back at the beginning with the overloaded version 🤣. After all this discussion I'm honestly contemplating it as a viable alternative.
I could try to take on the partial updates some time later on, if not done by then.
Well, it is worth exploring all alternatives, but at the end of the day, specially named keys might just be the solution.
Thanks everybody for the feedback. I tried multiple implementations and settled on @icyJoseph's idea:
What about assign(update).transition(event)?
(The only difference is that I'm using send
instead of transition
for consistency.)
send
is also still passed as an argument to effect. @RichieAHB, I understand your concerns about having multiple ways to call send
, and I gave quite a bit of thought to it, but I think it's worth it to keep the API compatible since there are already people using this library.
The PR is open here (still work in progress) #37: It also covers a new event system that the user can use to pass arbitrary values into the State Machine.
I can see a good deal of consideration has gone into this and I think the result looks nice and reads well. I also agree with your pragmatic decision around having multiple ways to call send
if it feels like the right trade off - it’s definitely a worthy aim to keep existing APIs stable 👍
My only question is what would be the plan around batching state updates down this path (as mentioned above)? This kind of locks you out of it unless you delay evaluation of the argument passed to assign
until after the chained transition
/ send
is called; but I’m not sure this is possible given assign
maybe validly called on it’s own. You may have decided that this isn’t the right time for this change, but thought I’d raise as I hadn’t seen it addressed!
Yes, I forgot to mention that, thanks for the reminder.
I actually implemented batch updates while experimenting with one of the APIs. It's definitely trickier with this selected API, so for now I just moved forward with the API change and batch updates will follow.
Request for comments for new
update
signature inside effect.@RunDevelopment, @dani-mp, @RichieAHB, @icyJoseph, and anyone interested.
Problem Statement
One issue we currently have within effects is that even though
send
andupdate
are different functions, they're frequently used together (update the context, then trigger a transition). E.g.:Proposal (Updated)
The proposal is to pass a new
assign
parameter toeffect
. It's a function that takes an Object as argument with the type:The above fetch example would look like this:
Proposal (Old)
The proposal is to keep passing both
send
andupdate
(as well asevent
, in the future) toeffect
, BUT with one change:update
will also returnsend
, so you can couple updating the context and sending event on a single line:Granted, it might look a little weird at first, but it has a few advantages:
send
or justupdate
(keep a simple API and backward compatible)Alternative Considered
We could remove the
update
method fromeffect
and overloadsend
to also update the context:I see a few disadvantages here:
send
method insideeffect
would be different from thesend
returned when calling useStateMachine. We might need to come up with different nomenclature or incur the risk of making the API confusing.Thoughts? Ideas? Comments?