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

Include TTypestate for methods #1090

Merged
merged 6 commits into from
Mar 23, 2020

Conversation

rjdestigter
Copy link
Contributor

This fixes issues where the state returned by useMachine looses the TTypestate information

@changeset-bot
Copy link

changeset-bot bot commented Mar 22, 2020

🦋 Changeset is good to go

Latest commit: 3d6c015

We got this.

This PR includes changesets to release 1 package
Name Type
xstate Patch

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 22, 2020

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:

Sandbox Source
peaceful-violet-3cviw Configuration
prod-glitter-ek9kr Configuration

@rjdestigter
Copy link
Contributor Author

I want to add tests but I'm having trouble getting the current set of tests to run without failing. I also ran npx lerna bootstrap rather than yarn but I'm not sure if that was correct.

@davidkpiano
Copy link
Member

@rjdestigter Seems like tests passed, did you run yarn and then yarn test:core in the root dir?

@rjdestigter
Copy link
Contributor Author

Yeah, I did and that was successful.

@rjdestigter
Copy link
Contributor Author

If I want to test one or more of the other packages. Do I manually run yarn inside their respective root folder?

@davidkpiano
Copy link
Member

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 yarn test:core

@rjdestigter
Copy link
Contributor Author

I think this might be lerna + yarn + windows issue. node_modules at the root is linking to packages/core but running npm test --prefix ./packages/xstate-react fails due to xstate not being found/resolved.

@Andarist
Copy link
Member

@rjdestigter you can run yarn dev in the root to run all tests and tsc at the same time, this should resolve all issues regarding some packages not being found etc

@@ -57,3 +64,52 @@ describe('useService', () => {
render(<Todo index={0} />);
});
});

describe('useMachine', () => {
it('should preserve typestate information.', () => {
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Member

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!

Copy link
Contributor Author

@rjdestigter rjdestigter Mar 22, 2020

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

@rjdestigter rjdestigter marked this pull request as ready for review March 22, 2020 20:45
@rjdestigter
Copy link
Contributor Author

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., useMachine.

@davidkpiano davidkpiano merged commit 6b3953d into statelyai:master Mar 23, 2020
@github-actions github-actions bot mentioned this pull request Mar 23, 2020
@rjdestigter rjdestigter deleted the forward-typestate-type branch March 23, 2020 14:51
@github-actions github-actions bot mentioned this pull request Apr 3, 2020
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.

None yet

3 participants