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

feat: state value added to transition type #4043

Merged
merged 6 commits into from
Jun 6, 2023
Merged

feat: state value added to transition type #4043

merged 6 commits into from
Jun 6, 2023

Conversation

ksv90
Copy link
Contributor

@ksv90 ksv90 commented May 26, 2023

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented May 26, 2023

🦋 Changeset detected

Latest commit: 55cbfd7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@xstate/fsm 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

@ghost
Copy link

ghost commented May 26, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 26, 2023

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 55cbfd7:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration

'@xstate/fsm': patch
---

state value added to transition type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ksv90! Thanks for making your first contribution to @statelyai ❤️

Changesets generate changelog entries for users so you want to think like a user when writing your changesets.

If you're a user reading a changelog entry, you'd want to know things like:

  • What is the change?
  • Why is the change important?
  • How do I make use of the change?

Your changeset isn't technically wrong, it just doesn't help users understand or get excited about the change that you made here!

| {
target?: string;
target?: TStateValue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly does this solve for you? Could you share some code that is affected by this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Andarist
I decided that it would be better to show in dynamics. This file describes the state machine. The creation takes place elsewhere, here I wanted to show the development process. Please ask more questions if you need to explain in more detail what is happening in the video.

2023-05-27.13-10-34.mp4
import { StateMachine } from '@xstate/fsm';

export type TestContext = {
  readonly value: number;
};

export type TestStateValue = 'init' | 'loading' | 'idle' | 'process';

export type TestStateMachine = StateMachine.Config<
  TestContext,
  { type: 'NEXT' | 'STOP' | 'COMPLETE' },
  { value: TestStateValue; context: TestContext }
>;

export const createStateMachine = (): TestStateMachine => ({
  context: { value: 42 },
  initial: 'init',
  states: {
    init: {
      on: {
        COMPLETE: 'loading',
      },
    },
    loading: {
      on: {
        COMPLETE: { target: 'idle' },
      },
    },
    idle: {
      on: {
        NEXT: 'process',
      },
    },
    process: {
      on: {
        STOP: { target: 'idle' },
      },
    },
  },
});

@ksv90
Copy link
Contributor Author

ksv90 commented May 29, 2023

Please write if these changes cannot be added at this time.

@Andarist
Copy link
Member

I'm OK with the change for the current major version of @xstate/fsm but I'm not yet sure if this will stay here in the next major version.

We are exploring ways to have stricter config types both for xstate and @xstate/fsm. I think though that you shouldn't have to declare your state names upfront - that's redundant information if the same could be inferred from the passed-in config. To rephrase - we'd like to get you the same auto-completion benefits and strict types for initial/target but without you passing us any Typestate

@ksv90
Copy link
Contributor Author

ksv90 commented May 30, 2023

Thank you for the clarification. I want to add that in our case, we know all the names of the states before writing the code, since we design the state machine in advance. And the description of all states in a separate place (in this case, in the type) is an additional check so that we do not forget anything and describe all relationships. It becomes difficult to track if there are many states.

Perhaps in other situations it is really redundant. But then we will not have auto-completion until we have described the next state (otherwise the typescript will not know about this state) and we will have to return to writing each state again after that.

@ksv90
Copy link
Contributor Author

ksv90 commented Jun 4, 2023

Is there any certainty on this issue?

@Andarist
Copy link
Member

Andarist commented Jun 4, 2023

I'll land this tomorrow.

@Andarist
Copy link
Member

Andarist commented Jun 5, 2023

I want to add that in our case, we know all the names of the states before writing the code, since we design the state machine in advance. And the description of all states in a separate place (in this case, in the type) is an additional check so that we do not forget anything and describe all relationships. It becomes difficult to track if there are many states.

I don't consider this to be the primary way of authoring our state machines. Note that I'm not saying that it's bad or anything like that, it's just that usually state names come from the state machine itself and are not known ahead of time. For that reason, we plan to optimize for this workflow and use case.

I think though that there are ways to support your workflow as well. There are multiple ways to achieve this kind of extra validation that you are looking for. It's even possible to achieve this at type-level to get autocompletions and all. A short demo of this could look like this (TS playground):

type StateNames = "idle" | "loading" | "done";

type AOTSimpleSchema<T extends string> = {
  initial: T;
  states: Record<T, { on: Record<string, T> }>;
};

declare function createMachine<const TConfig>(config: TConfig): void;

createMachine({
  initial: "idle",
  states: {
    idle: {
      on: {
        EV: "done",
      },
    },
  },
} satisfies AOTSimpleSchema<StateNames>);

) => this is TState extends { value: TSV }
? TState & { value: TSV }
: never;
) => this is { value: TSV; context: TContext };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what motivated this change? could you give an example of what didn't work before this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since TSV was extended with TSstate['value'] this worked well for a true expression. But with a false expression, the context became type "never". So it was not possible to check multiple states:
if(state.matches('idle')) {}
else if(state.matches('latest')) {}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a test case for this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to add new examples there. This test passes in the current implementation because the value is of type any. You can pass the number 12 to the match function and it will work. But I narrowed the value down to a string, so the typescript started swearing at the second branch of the condition. Then I changed the types for the correct behavior.

@Andarist Andarist merged commit bc1799b into statelyai:main Jun 6, 2023
2 of 3 checks passed
@github-actions github-actions bot mentioned this pull request Jun 6, 2023
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