-
Notifications
You must be signed in to change notification settings - Fork 47
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
RFC: New effect & update Signature #35
Comments
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 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 ( |
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 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.
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:
Agreed. It was there for backwards compatibility - but it does add confusion
Even though I do think this assumption is true, I'd rather provide an API that lets the user do both.
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 #33? |
I think the above makes sense. There are a few points that it’s seems worth calling out:
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
|
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
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:
(The only difference is that I'm using
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 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 |
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?
The text was updated successfully, but these errors were encountered: