-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
refactor: 🏷️ stronger types for StateValue
when using Typegen
#3342
Conversation
|
👇 Click on the image for a new way to code review
Legend |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b2a466f:
|
lol @erikras just literally told me he wanted this when we went out for sushi |
jealous! 🍣 I'm hoping this will work..! |
packages/core/src/StateNode.ts
Outdated
@@ -780,8 +782,8 @@ class StateNode< | |||
|
|||
return next; | |||
} | |||
private transitionCompoundNode( | |||
stateValue: StateValueMap, | |||
private transitionCompoundNode<TResolvedTypesMeta>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the TResolvedTypesMeta
"flow" here from the outer type params? Right now I think that TResolvedTypesMeta
should be the same across this whole class - but in here, we allow it to be different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're 100% correct - great catch! should be fixed here: b2a466f
I think, but that would have to be verified, that reusing q: why do you want to use |
Sure thing! I could create a PR to add something like What would that look like though? How would it be different to
Basically, just for better type safety. I created a small repro here to show an example of when we get a type error when we should not: https://codesandbox.io/s/github/brunocrosier/test-xstate-typegen Happy to explain in more detail if that's unclear! |
One important difference is that only the "object syntax" is valid for Another one is that |
OK, I will see if I can create a PR in |
It would still be pretty interesting to see some real-life examples of how you are using |
Unfortunately I don't have any examples apart from that minimal CodeSandbox, sorry! So far I've only played around with XState in personal projects - but I guess that stronger types are always better, right? Maybe @erikras has some examples of potential use-cases? Added PR here: statelyai/xstate-tools#160 |
Superseded by #4498 |
Demo of
state.value
having loose types while using Typegen: https://codesandbox.io/s/github/brunocrosier/test-xstate-typegenBorrowing the type from: https://github.com/statelyai/xstate/blob/main/packages/core/src/State.ts#L321