-
Notifications
You must be signed in to change notification settings - Fork 49
Porting txstate #36
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
Porting txstate #36
Conversation
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
|
@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 |
To answer your questions -
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.
It's inspired from ts-toolbelt. The idea is all types under
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. |
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 |
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 |
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 🙏😅 |
No, you didn't sound rude at all! Ok, so we have everything merged on the 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. |
Cool, going to work on types now hopefully I'll update the branch soon.
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.
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 |
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
So now just |
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. |
Yeah no worries take your time it's unpaid work after all ;) |
@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 I plan to start tackling the code to use primitive types in the end of next week. |
Nice, I too will complete the rest todos soonish!
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 |
Wait, I don't understand. Is that for states with no transitions? |
Yep, if |
I kind of understand the issue, but it's definitely beyond my TS knowledge. Question, instead of the phantom |
Yep, both would work. Let me know if you want to go with any one of those. |
"type: final" would have different implications, so I Think the least bad would be an "on: null". |
Also in case you suggested |
I think we could keep both. |
I think this is ready for final review and merging! Also I kept it as 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. |
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 |
Haha thanks a lot, my pleasure! And thanks to you too for being so welcoming!
Great, I think a week would be sufficient time.
Yep, feel free to ask questions if you're trying to reuse the types. |
Wait, it seems to be something wrong: I can't find the |
That's right I replaced it with a more minimal Also there's some error in exit events, I'll fix it tomo. |
Also you haven't addressed this. And I have added some tasks here too. (I'll probably make type changes in this branch) |
Should we close this PR in favor of #56 ? |
Yep we should, closing. |
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. |
We'd document both, I'll add it to the "downsides" section of my release article.
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 let d = createDefintion({ ... }) // more readable errors and no need for "as const"
useStateMachine(d) Wdyt? |
Superseded by #56
Todos
on
_
bug - right now we need to add_: null
is a state node only has a effect, I think the problem is at TS's end though will have to find a workaround. - update: made iton: null
still haven't investigated yet to see if we can fix it completelywe can't fix it, so pending is to add a custom error to tell user to addon: null
expectCompletionsToBe
,expectQueryTextToInclude
ie automated testing with language server(note to self)nope, doesn't make intellisense queries more readabletype _Effect<D, P> = Effect<X, Y, X>
, etcadd jsdocapparently there is no effect in adding jsdocs to the types, they get lost and the user can't see themstate
Original Description
Hi Cassio! Here's a little rough PR for porting txstate for better types.
Improvements
Way better error messages
Scenario 1
Before -
After -


Scenario 2
Before -
After -


And many more scenarios that I don't have handy...
Typed guards hence inferring context typestates
Example
Other notes