RFC: New effect & update Signature about usestatemachine HOT 16 CLOSED

Updated the proposal

from usestatemachine.

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.

from usestatemachine.

Work in progress branch: https://github.com/cassiozen/useStateMachine/tree/updatedSend

from usestatemachine.

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!

from usestatemachine.

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.

from usestatemachine.

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.

from usestatemachine.

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'});
}

from usestatemachine.

I like it, but where would you put context to cover #33?

from usestatemachine.

I think the above makes sense. There are a few points that it’s seems worth calling out:

• As mentioned above for the last alternative, this does have a different signature from the “send” that is returned from the hook, although given it also has a different name this seems like less of an issue now
• I feel like transition is closer to the semantics than assign - but that is personal preference!
• The most major point I can think of is that, given this is an API change and #33 is also suggesting an API change, it may be worth considering those things together to avoid two API changes. That might be out of scope here but it may make the 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!)

from usestatemachine.

I'm tending to not add a getContext to effect for the next version:

• #33 core issue is about passing arbitrary data to an effect, which will already be possible through the new event system (#31).
• it might encourage "fat" effects, when we want people to also use guards where appropriate.

I can certainly be convinced otherwise, but for now my idea is to keep the updater function for the context.

from usestatemachine.

Assuming that there is no requirement to change the signature in the next version then personally I think this is a good change!

from usestatemachine.

I think the unification under assign looks very good. Here's just a few things floating around in my head when I see it:

• We'd like to simply return a partial update to the context, that is 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?
• 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: 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)
• What about assign(update).transition(event)?

from usestatemachine.

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.

from usestatemachine.

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.

from usestatemachine.

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!

from usestatemachine.

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.

from usestatemachine.