-
-
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
Include TTypestate
for methods
#1090
Include TTypestate
for methods
#1090
Conversation
🦋 Changeset is good to goLatest commit: 3d6c015 We got this. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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 3d6c015:
|
I want to add tests but I'm having trouble getting the current set of tests to run without failing. I also ran |
@rjdestigter Seems like tests passed, did you run |
Yeah, I did and that was successful. |
If I want to test one or more of the other packages. Do I manually run yarn inside their respective root folder? |
Use the same strategy as |
I think this might be lerna + yarn + windows issue. node_modules at the root is linking to packages/core but running |
@rjdestigter you can run |
@@ -57,3 +64,52 @@ describe('useService', () => { | |||
render(<Todo index={0} />); | |||
}); | |||
}); | |||
|
|||
describe('useMachine', () => { | |||
it('should preserve typestate information.', () => { |
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.
I'm wondering - so far you have only changed xstate
's source files, but you have added a test to @xstate/react
. Any particular reason behind this? Should we add similar tests to xstate
? Were your changes essential for @xstate/react
?
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.
The goal of the changes where to preserve typestate for components using useMachine
.
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.
We should. I'm working on some additional typestate related tests for core
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.
The goal of the changes where to preserve typestate for components using useMachine.
This was probably already fixed by this commit. Notice how the return type of useMachine
is already typed with type states in mind, so your changes had no effect on it.
It's great to have tests to cover this now though!
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.
👍 I've added tests for core
These changes could be a breaking change for anyone using typestates to define/create their machine but then ignoring them after using any of the methods or functions using these methods, e.g., |
This fixes issues where the state returned by
useMachine
looses theTTypestate
information