Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

brunocrosier
Copy link
Contributor

@changeset-bot
Copy link

changeset-bot bot commented May 25, 2022

⚠️ No Changeset found

Latest commit: b2a466f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ghost
Copy link

ghost commented May 25, 2022

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 25, 2022

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:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration
test-xstate-typegen PR

@brunocrosier brunocrosier marked this pull request as draft May 25, 2022 21:01
@mattpocock
Copy link
Contributor

lol @erikras just literally told me he wanted this when we went out for sushi

@brunocrosier
Copy link
Contributor Author

lol @erikras just literally told me he wanted this when we went out for sushi

jealous! 🍣

I'm hoping this will work..!

@brunocrosier brunocrosier marked this pull request as ready for review May 25, 2022 21:45
@mattpocock mattpocock requested a review from Andarist May 25, 2022 22:59
@@ -780,8 +782,8 @@ class StateNode<

return next;
}
private transitionCompoundNode(
stateValue: StateValueMap,
private transitionCompoundNode<TResolvedTypesMeta>(
Copy link
Member

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

Copy link
Contributor Author

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

@Andarist
Copy link
Member

Andarist commented Jun 1, 2022

I think, but that would have to be verified, that reusing matchesStates is not good enough here because it accepts some object~ flavors that are not correct for the state.value.

q: why do you want to use state.value in the first place? I'm not saying that it's not OK - I just want to understand the context behind the request etc. I think that state.value & state.matches are so close to each other that I wonder if there is a value in having both of them in the API.

@brunocrosier
Copy link
Contributor Author

I think, but that would have to be verified, that reusing matchesStates is not good enough here because it accepts some object~ flavors that are not correct for the state.value.

Sure thing! I could create a PR to add something like stateValues here: https://github.com/statelyai/xstate-tools/blob/e2d7a2b498fb4d3a666c434748d678ce6ca5f8e7/packages/shared/src/getTypegenOutput.ts#L164 ?

What would that look like though? How would it be different to matchesStates ?

q: why do you want to use state.value in the first place? I'm not saying that it's not OK - I just want to understand the context behind the request etc. I think that state.value & state.matches are so close to each other that I wonder if there is a value in having both of them in the API.

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!

@Andarist
Copy link
Member

Andarist commented Jun 7, 2022

What would that look like though? How would it be different to matchesStates ?

One important difference is that only the "object syntax" is valid for state.value (+ string values for top-level atomic state) but a "dotted" notation is not valid:
https://github.com/statelyai/xstate-tools/blob/041846a873d58247fe17206f1e6149916203b449/packages/shared/src/getTypegenOutput.ts#L93-L97

Another one is that .matches always mark properties as optional:
https://github.com/statelyai/xstate-tools/blob/041846a873d58247fe17206f1e6149916203b449/packages/shared/src/getStateMatchesObjectSyntax.ts#L24
but for parallel regions in the .value this probably wouldn't be true, those should actually always be required.

@brunocrosier
Copy link
Contributor Author

OK, I will see if I can create a PR in xstate-tools to add stateValues with the requirements that you mentioned 🙂

@Andarist
Copy link
Member

Andarist commented Jun 8, 2022

It would still be pretty interesting to see some real-life examples of how you are using state.value in your code.

@brunocrosier
Copy link
Contributor Author

It would still be pretty interesting to see some real-life examples of how you are using state.value in your code.

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

@davidkpiano
Copy link
Member

Superseded by #4498

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants