cassiozen / useStateMachine

The <1 kb state machine hook for React
MIT License
2.38k stars 47 forks source link

Api changes for `sendT` #72

Closed devanshj closed 2 years ago

devanshj commented 3 years ago
machine = { ...machineInstant, send }
Machine.State -> Machine
machineState -> machineInstant
machineState.value -> machineInstant.state
Machine.StateValue -> Machine.State
cassiozen commented 3 years ago

Interesting, not what I was expecting. I thought we were going for {machine, state} = useStateMachine(...) instead of {state, context, nextEvents, send} = useStateMachine(...).

devanshj commented 3 years ago

Ha I thought it was apparent in the article haha. To explain why it's like that we need the discriminating property one level deep make the narrowing happen, hence machine.state === "editing" && machine.send("DISCARD") would work.

With your version, state === "editing" && machine.send("DISCARD") won't narrow machine something.state === "editing" && something.machine.send("DISCARD") would but the whole thing looks too complex.

Moreover with shouldSendT we might narrow the machine via this type predicate as a discriminating union would no longer work so to make machine.state === "editing" && machine.shouldSendT("POST") && machine.sendT("POST") work shouldSendT might be something like...

shouldSendT(this: X, event: Y): this is Z

And hence the users should probably not use destructuring as it might make the type predicate not work.

Moreover I'm not sure if machine.state === "editing" will suffice we might have to opt for machine.hasState("editing") with the same this predicate.

I haven't really tried any of this out though haha probably I should.

cassiozen commented 3 years ago

Moreover I'm not sure if machine.state === "editing" will suffice we might have to opt for machine.hasState("editing") with the same this predicate. I haven't really tried any of this out though haha probably I should.

Can you try it before we merge?

Another question: Can I update the existing examples to use rest in the destructuring? Like:

const {send, ...machine} = useStateMachine({...})
devanshj commented 3 years ago

Can you try it before we merge?

Yep will do.

Can I update the existing examples to use rest in the destructuring?

I'd say prefer not to, because in typescript's eyes send (and shouldSend) is somewhat on the prototype (hence should not be destructured) as it would probably leverage this. I can't comment concretely before I have tried things. I'll give an update after I've tried.

devanshj commented 3 years ago

There... Will have to introduce hasState to have these error messages (as opposed to simply not accepting "Y")

image

And destructuring obviously won't work nor typescript would allow it at places I've defined this

image

Though we can't have the error messages that I wanted to have like Error: Y can be only sent when in b or something like that because while the time of showing error the argument is not inferred as "Y" so to have error like those we'd tell users to write sendT.debug("Y") which would return the error as a string and then sendT would simply not suggest "Y" nor it would show errors.

Anyway, the point was to see if we can leverage this guard and we can, then in that case some of these become methods on prototypes for TypeScript (we don't actually need to have them on the prototype in runtime) so we can't use destructuring as it won't work as shown above.

EDIT: Wait given that folks will have to use hasState to leverage sendT should we add it already? Or add it with sendT?

cassiozen commented 2 years ago

It feels like hasState has a lot of value even without sendT, so I think we should move forward and implement it.

devanshj commented 2 years ago

We should implement hasState but it has literally no value without sendT because m.hasState(x) is same as writing m.state === x. The only time m.hasState(x) and m.state === x would differ is with usage of sendT because hasState narrows m but m.state === x doesn't (sufficiently enough to make sendT work).

The reason I was suggesting we should already have hasState is so that users start writing m.hasState(x) instead of m.state === x so that when we have sendT they can leverage it right away instead of first migrating m.state === x to m.hasState(x).

So I guess first we should merge this and then I'll open a separate PR for hasState? Actually before hasState I'd open a PR for createDefinition so as to close #78

Edit: Oh I realized this has conflicts haha will resolve

cassiozen commented 2 years ago

Ohh, got it. Yeah, I think it might make sense to release it with sendT then.

devanshj commented 2 years ago

Will resume working on the documentation once this (that includes #76) is released so as to not redo the work as the api has changed

cassiozen commented 2 years ago

Just released it, @devanshj

devanshj commented 2 years ago

Great! Could you merge and release this PR too? (As this PR has the finalized api to be documented)

cassiozen commented 2 years ago

Hey @devanshj,

Since I've been stuck as work for a while and the library have been steadily increasing, I reverted back and released Beta 3 as 1.0.0.

I'm reopening this, we can further discuss releasing it as 2.0.0

devanshj commented 2 years ago

Sure no problems, indeed we can release it as v2.

I've been busy too haha, looking forward to continue working on this library!