cassiozen / useStateMachine

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

Porting txstate #36

Closed devanshj closed 3 years ago

devanshj commented 3 years ago

Superseded by #56

Todos

Original Description

Hi Cassio! Here's a little rough PR for porting txstate for better types.

Improvements

Way better error messages

Scenario 1
Before - ![image](https://user-images.githubusercontent.com/30295578/120134395-9ec33280-c1eb-11eb-9947-85f27a1c6810.png) ![image](https://user-images.githubusercontent.com/30295578/120135034-ff9f3a80-c1ec-11eb-9987-177ca3feae4f.png) After - ![image](https://user-images.githubusercontent.com/30295578/120134907-bea72600-c1ec-11eb-98c5-e745536e4ec3.png) ![image](https://user-images.githubusercontent.com/30295578/120134470-cadeb380-c1eb-11eb-91b2-55b5a819bd57.png)
Scenario 2
Before - ![image](https://user-images.githubusercontent.com/30295578/120134978-e3030280-c1ec-11eb-9e4d-ebba09450f00.png) ![image](https://user-images.githubusercontent.com/30295578/120135082-1c3b7280-c1ed-11eb-8071-af9223acfc0c.png) After - ![image](https://user-images.githubusercontent.com/30295578/120135119-2b222500-c1ed-11eb-8e29-c39a07555784.png) ![image](https://user-images.githubusercontent.com/30295578/120135162-42611280-c1ed-11eb-9915-61f0826d7d40.png)

And many more scenarios that I don't have handy...

Typed guards hence inferring context typestates

Example ```ts // example picked from xstate docs let [flightMachine] = useStateMachine({ trip: 'oneWay' })({ initial: 'editing', states: { editing: { on: { SUBMIT: { target: 'submitted', guard: (context): context is FlightMachineSubmittedContext => { if (context.trip === 'oneWay') { return !!context.startDate; } else { return ( !!context.startDate && !!context.returnDate && context.returnDate > context.startDate ); } } } } }, submitted: {} } }) type FlightMachineEditingContext = { trip: 'oneWay' | 'roundTrip' , startDate?: Date , returnDate?: Date } type FlightMachineSubmittedContext = | { trip: 'oneWay' , startDate: Date } | { trip: 'roundTrip' , startDate: Date , returnDate: Date } type FlightMachineContext = | FlightMachineEditingContext | FlightMachineSubmittedContext expectType(flightMachine.context); if (flightMachine.value === 'submitted') { expectType(flightMachine.context); if (flightMachine.context.trip === 'oneWay') { expectError(flightMachine.context.returnDate) } } ```

Other notes

cassiozen commented 3 years ago

Hi @devanshj - I'm still trying to dive into this, I have a couple of questions if you don't mind:

1 - You left pretty much all the types intact in index.tsx - but I'm assuming these aren't needed anymore? 2 - You have some helper types (great stuff, BTW) under the namespaces O, L, F and A. Why are they under so many namespaces? 3 - I'll be honest with you - I really want to use this, but I don't feel like I can contribute right now to your project. So, my question is - can you keep helping with changes until we push version 1.0? Here are the expected changes:

devanshj commented 3 years ago

@cassiozen Before I reply to you I think there's a big misunderstanding xD You realise this PR has nothing to do with txstate per se, I'm calling it "Porting txstate" because I'm using the techniques used in txstate. This PR and txstate are TOTALLY independent. I linked that document because it'll help you understand this PR somewhat.

So I'm not asking you to contribute to txstate, this PR is to be treated independent as if txstate does not exist.

Did you already know all this or there was some misunderstanding here? xD

devanshj commented 3 years ago

To answer your questions -

You left pretty much all the types intact in index.tsx - but I'm assuming these aren't needed anymore?

The thing is I've kept the user facing types and types for implementation different. The reason is you can't use the one's I've added (I'm calling it user facing) to type implementation details (Let me know if you want me to elaborate on this). So to answer your question we'll be needing both of them.

You have some helper types (great stuff, BTW) under the namespaces O, L, F and A. Why are they under so many namespaces?

It's inspired from ts-toolbelt. The idea is all types under O namespace serve as operations on object, L on arrays/tuples, F in functions and A on all types. It's just a matter of convention and preference.

can you keep helping with changes until we push version 1.0?

More or less yes, I can't give a word because I haven't checked the issues yet but it's mostly a yes from me. The worst case scenario is we'd have to compromise on types in scenarios where it's not possible to type them. I'll give you a concrete answer once I check issues you've linked.

cassiozen commented 3 years ago

Yes, I understand that, my writing was confusing. Whenever I say "contributing to txstate" I mean "contributing to 'types.ts' inside this project".

devanshj commented 3 years ago

Yes, I understand that, my writing was confusing. Whenever I say "contributing to txstate" I mean "contributing to 'types.ts' inside this project".

Ah gotcha, I too thought maybe you meant this but wasn't sure xD

devanshj commented 3 years ago

I'll give you a concrete answer once I check issues you've linked.

I went through all four issues seems doable to me. So it's a yes I'll keep updating this PR (or opening new one's if we're merging feature by feature) till 1.0 (or even forever if time permits :P)

Just there's one issue that the types assume that users are writing the machine definition (config) in place as literal so that the type inferred of the definition is of unit type (the type which can be assigned only one value), as in anything like the following might create an issue...

let m = { ... } // the definition here
useStateMachine()(m)
// won't work because the types of `m` are already inferred
// so we can't have contextual inference which might cause problems
// because things in `m` would be inferred as `string` (which makes the type non-unit)
// instead of literals like "red"` or whatever

useStateMachine()({
  initial: Math.random() > 0.5 ? "a" : "b", 
  states: { a: {}, b: {} }
})
// might be a problem because the type of machine definition inferred
// is not unit it's a union of two types one with "a" and other with "b".

I haven't investigated how to go about cases like this but this could be a potential blocker for this PR, imo the pros are way more than this one con, but you're the boss :P

devanshj commented 3 years ago

@cassiozen Before I reply to you I think there's a big misunderstanding xD You realise this PR has nothing to do with txstate per se, I'm calling it "Porting txstate" because I'm using the techniques used in txstate. This PR and txstate are TOTALLY independent. I linked that document because it'll help you understand this PR somewhat.

So I'm not asking you to contribute to txstate, this PR is to be treated independent as if txstate does not exist.

Did you already know all this or there was some misunderstanding here? xD

Also @cassiozen I realized this sounded a little rude when I reread later, I wasn't in the best headspace when I replied, my sincere apologies 🙏😅

cassiozen commented 3 years ago

No, you didn't sound rude at all!

Ok, so we have everything merged on the main branch for the 1.0 release, whenever you have time to update the types, we can release it as is and leave hierarchical state machines for next on queue.

A few comments on your responses:

User facing types: Ok, perfect. It worries me a little bit that future developments (like the hierarchical state machines) will have to be done on both sides. Maybe there is a way for me to reuse some of the ported txstate types as user-facing?

Namespaces: Got it, makes sense. In the current types, I moved from single-letter to fully named generics - Do you have strong feelings about this? I could refactor the types.ts names, keeping the same structure.

devanshj commented 3 years ago

Cool, going to work on types now hopefully I'll update the branch soon.

Maybe there is a way for me to reuse some of the ported txstate types as user-facing?

In theory, yes. Thought it'll be extremely verbose and perhaps would do more harm than good, like you'd need more assertions in your implementations.

By verbose I mean it'll look like...

const someImplementationDetail = <N, C>(node: MachineDefinition.StateNode<N, C, []>) => {}

Or in future versions like...

const someImplementationDetail = <N, C>(node: MachineDefinition.StateNode<N, C, MachineDefinition.Precomputed<N>, []>) => {}

Meaning there would be no handy generic-free type that you'd be able to assign to a variable and call it a day.

Instead what I would suggest is dumb down the implementation types. Make them generic-free and just "runtime equivalent" like so...

interface StateNode {
  initial: string,
  states: Record<string, StateNode>
}

Because there isn't a point of having sophisticated types for implementation, in my opinion. If you could manage to have implementation types as good as user facing, well and good, though I personally wouldn't care so much.

Do you have strong feelings about this?

I kinda have, though we're more or less on the same page, I too am for fully named generic but in few cases I prefer them to be single letter. For example right now more or less all types start with D, C, S or at least D, C. It wouldn't make sense to write Definition, Context, Self all the time. Another instance is for the types inside O, L, etc namespaces there too it's obvious what single generics types mean. Otherwise I too am for fully named generics like you'll see States, Gaurd, etc in the types.ts. Though in some places they are a little ambiguous like Ts, S, etc. and I'll refactor them myself.

devanshj commented 3 years ago

Hi @cassiozen here's some wip, I'll keep updating the original description and ping you whenever I do it (not sure if github notifies for new commits in a PR), still 15% work more to go

Could you go through the test when you get time? I'm doing some fancy stuff (you'll know when you see the tests :P) so want to make sure that's in line with you.

Also I've bailed out of tsd because

  1. expectError doesn't give me live feedback whereas @ts-expect-error does, moreover it isn't reasonable to have expected red underlines floating around, it's like now you don't know if something is off even if there's red shit in your ide because it could include the expected ones.
  2. expectType too doesn't give live feedback (the way it's typed it only checks for subtyping afaik so you'll have to run tsd to actually see the results) whereas A.test(A.areEqual<A, B>()) does give me live feedback

So now just tsc tests the types

cassiozen commented 3 years ago

Yes, I'm super curious to check it out. I'm a little blocked because of worked but will check as soon as possible.

As for TSD, that's totally fine. I wanted a way to test the types and make sure we make no regressions during new development - but if you can test them using something else that's fine.

devanshj commented 3 years ago

Yeah no worries take your time it's unpaid work after all ;)

cassiozen commented 3 years ago

@devanshj - I just went through the tests and through the initial work on types.ts. So far everything looks good. I love how you use string literals instead of never to represent errors in the type level.

I plan to start tackling the code to use primitive types in the end of next week.

devanshj commented 3 years ago

Nice, I too will complete the rest todos soonish!

I love how you use string literals instead of never to represent errors in the type level.

Haha ikr thanks!

Also if I can't find a workaround for the "" thing would it be a blocker? Or it's fine to expect users to write `: null? Or would you want to compromise on some features to have users not write_: null`?

cassiozen commented 3 years ago

Wait, I don't understand. Is that for states with no transitions?

devanshj commented 3 years ago

Yep, if effect is the only property (ie no on) you'd have to add a phantom _: null to make it work 😬 (related ts issue)

cassiozen commented 3 years ago

I kind of understand the issue, but it's definitely beyond my TS knowledge. Question, instead of the phantom _, does it work if the user is required to provide a null on key, or a final state like type: 'final'?

devanshj commented 3 years ago

Yep, both would work. Let me know if you want to go with any one of those.

cassiozen commented 3 years ago

"type: final" would have different implications, so I Think the least bad would be an "on: null".

devanshj commented 3 years ago

Also in case you suggested on: null thinking it has to be null then no, on: {} works too. Should we keep that? Or both?

cassiozen commented 3 years ago

I think we could keep both.

devanshj commented 3 years ago

I think this is ready for final review and merging!

Also I kept it as on: {} and not on: null, because allowing the latter seems to be that we're changing runtime semantics unnecessarily for type-land issues. And I also added a custom error that instructs the user to add on: {} when required you can see the tests for more.

Don't announce v1.0 yet, I think I'm gonna make a nice document to show off all these features :P which can go with the announcement. I hope I get time and I'll let you know in either case.

Let me know if you'd first like to make the runtime implementation changes in a new branch and get it merged with main and then have me rebase it to that, I think that'd be more clean and then the CI would also pass on this PR.

cassiozen commented 3 years ago

That is awesome, @devanshj - you're the best!

So my plan is to release this as a beta for at least a week, so you'll have time to write the nice document!

I also want to do a clean-up pass on index.tsx to remove what now are unnecessary type definitions and either use primitives or try to use some TXState types passing some anys to fill in the generics.

devanshj commented 3 years ago

That is awesome, @devanshj - you're the best!

Haha thanks a lot, my pleasure! And thanks to you too for being so welcoming!

So my plan is to release this as a beta for at least a week, so you'll have time to write the nice document!

Great, I think a week would be sufficient time.

I also want to do a clean-up pass on index.tsx to remove what now are unnecessary type definitions and either use primitives or try to use some TXState types passing some anys to fill in the generics.

Yep, feel free to ask questions if you're trying to reuse the types.

cassiozen commented 3 years ago

Wait, it seems to be something wrong: I can't find the createSchema function...

devanshj commented 3 years ago

That's right I replaced it with a more minimal t function because the user is going to write them many times so it doesn't make sense to have such long name imo. Oh oops I realised I forgot to update the runtime tests haha.

Also there's some error in exit events, I'll fix it tomo.

devanshj commented 3 years ago

Also you haven't addressed this. And I have added some tasks here too. (I'll probably make type changes in this branch)

cassiozen commented 3 years ago

Should we close this PR in favor of #56 ?

devanshj commented 3 years ago

Yep we should, closing.

devanshj commented 3 years ago

Tho this is left here

cassiozen commented 3 years ago

Let me address those separately:

useStateMachine()({
  initial: Math.random() > 0.5 ? "a" : "b", 
  states: { a: {}, b: {} }
})
// might be a problem because the type of machine definition inferred
// is not unit it's a union of two types one with "a" and other with "b".

Absolutely OK.

let m = { ... } // the definition here
useStateMachine()(m)
// won't work because the types of `m` are already inferred
// so we can't have contextual inference which might cause problems
// because things in `m` would be inferred as `string` (which makes the type non-unit)
// instead of literals like "red"` or whatever

I'm ok with this one as well, though we might want to explicitly document this somewhere. Question: Would it work if the user defined the configuration externally but used as const everywhere?

devanshj commented 3 years ago

though we might want to explicitly document this somewhere.

We'd document both, I'll add it to the "downsides" section of my release article.

Would it work if the user defined the configuration externally but used as const everywhere?

Yep that'd work, though consequently the errors will be much harder to read.

What we can do is we'd also provide an identity function called createDefinition then users can use it like this -

let d = createDefintion({ ... }) // more readable errors and no need for "as const"
useStateMachine(d)

Wdyt?