Skip to content
This repository has been archived by the owner on Mar 5, 2023. It is now read-only.

Typestates? #27

Open
bard opened this issue Sep 14, 2020 · 34 comments
Open

Typestates? #27

bard opened this issue Sep 14, 2020 · 34 comments

Comments

@bard
Copy link

bard commented Sep 14, 2020

First: thanks for bringing more safety to the machines!

I was wondering whether support is planned for Typestates, to narrow context type inside statements such as if (state.matches('foo')) { ... }?

@mattpocock
Copy link
Owner

Thanks @bard!

Yes, we don't currently support this but we should. It will mean some added complexity but it's worth it to support this pattern.

@davidkpiano do you plan to support Typestates in V5?

@mattpocock
Copy link
Owner

@bard There are several improvements we could bring here. First, as you say, this pattern:

type TypeState = { value: 'green'; context: 'green-context' } | { value: 'red'; context: 'red-context' };

if (state.matches('green')) {
  // This should error, because we now know that context === 'green-context'
  state.context === 'red-context';
}

Also, in options passed to interpret, useMachine or the second argument of Machine/createMachine we should know exactly which state an action/service/guard occurs in, so we know the exact shape of the context.

@bard
Copy link
Author

bard commented Sep 14, 2020

Indeed, now that you mention it the second case is something I found myself wishing as well. I did work around the lack of it by e.g. casting event types, but I can easily see myself lose track of things in more complex machines and cast to the wrong type.

@mattpocock
Copy link
Owner

Definitely - the casting of event types is something we've hopefully solved already. We should aim to do no casting whatsoever inside any options passed

@mattpocock
Copy link
Owner

@bard I don't use Typestates day-to-day so I'll have some questions about usage.

Can you model nested states with Typestates? Or only the top-level? For instance, could you model this:

type State = { value: 'foo'; context: false } | { value: 'foo.child'; context: true };

@bard
Copy link
Author

bard commented Sep 14, 2020

I haven't dealt with that directly (I've used nested machines rather than nested states), but according to the Typestate section in the docs:

A Typestate is an interface consisting of two properties:

  • value - the state value of the typestate (compound states should be referenced using object syntax; e.g., { idle: 'error' } instead of "idle.error")
  • context - the narrowed context of the typestate when the state matches the given value

XState seems to use the { parentStateValue: childStateValue } format in a few places, so maybe "parentStateValue.childStateValue" is a bit against the grain?

@mattpocock
Copy link
Owner

XState seems to use the { parentStateValue: childStateValue } format in a few places, so maybe "parentStateValue.childStateValue" is a bit against the grain?

Yes, I think it's falling out of favour.

Thanks for your help 👍

@davidkpiano
Copy link

Yeah, V5 will enforce using objects instead, e.g., { parentStateValue: childStateValue }, and Typestates will be supported in V5.

However, Typestates (at least right now) should manually be specified, since they can't be easily inferred from the machine.

@mattpocock
Copy link
Owner

Yep, inferring Typestates is beyond our power but we can certainly support them.

@mattpocock
Copy link
Owner

mattpocock commented Sep 14, 2020

Got a POC working here:

#28

This should work with the current Typestate API. It doesn't yet handle the state.matches use case yet. Does XState handle these inferences natively?

@bard
Copy link
Author

bard commented Sep 14, 2020

Got a POC working here:

#28

This should work with the current Typestate API. It doesn't yet handle the state.matches use case yet. Does XState handle this natively?

interpret does:

  const service = interpret(appMachine).start()
  if (service.state.matches('idle')) {
    console.log(state)
/*
(property) Interpreter<AppContext, any, AppEvent, AppState>.state: State<AppContext, AppEvent, any, AppState> & State<{
    bar: string;
}, AppEvent, any, AppState> & {
    value: "idle";
}
*/
  }

useMachine however does not:

export const App = () => {
  const [state] = useMachine(appMachine)
  if (state.matches('idle')) {
    console.log(state)
/*
const state: State<AppContext, AppEvent, any, {
    value: any;
    context: AppContext;
}> & {
    value: string;
}
*/
  }

@mattpocock
Copy link
Owner

Interesting - this might be worth an issue on the xstate repo if there isn't one already.

Thanks, this gives me some more to go on.

@davidkpiano
Copy link

Are you using @xstate/react@next @bard ?

@bard
Copy link
Author

bard commented Sep 14, 2020

@davidkpiano no. I've reproduced it in this sandbox , forked from codesandbox.io's React+TypeScript sandbox — there's actually a typing issue even before any attempt at type narrowing — but the same works in this sandbox forked from XState's official React+TypeScript sandbox, including type narrowing. Not sure what's going on.

@bard
Copy link
Author

bard commented Sep 14, 2020

@davidkpiano oh, wait, is 1.0.0-rc.4 @next? If so, then it works with @next.

@mattpocock
Copy link
Owner

mattpocock commented Sep 14, 2020

@bard Added inferencing on state.matches for both in that PR.

Any chance you could check it out locally? You can run yarn x test:watch from root and edit the state machines in the examples folder to try and break it.

@bard
Copy link
Author

bard commented Sep 14, 2020

@mattpocock. Wow, that was fast. Going to try it soon!

@bard
Copy link
Author

bard commented Sep 15, 2020

I haven't been able to break it so far. Context is narrowed correctly and plenty other things that would have caused a runtime error fail early with a sweet type error (but you already knew that). I'm going to try and sketch a simple app to see how it fares.

Something (unrelated) I noticed is that type information in the editor occasionally gets out of sync, and e.g. service.state is recognized as any. Probably a side effect of recompiling in the background and the language server not picking the change, but restarting the language server takes care of it and it's only a minor annoyance compared to the benefit.

@mattpocock
Copy link
Owner

@bard That 'any' thing is to do with the test suite - it rimraf's the entire node_modules dir, npm installs it again and rebuilds the types from scratch. This shouldn't happen during watching.

Thanks for testing this out, much appreciated.

Do you need this pushed to a next branch so you can install it easier?

@mattpocock
Copy link
Owner

One thing to test actually - are you getting type checking when you use state.matches? It should be type checking against all valid values.

@bard
Copy link
Author

bard commented Sep 15, 2020

@bard That 'any' thing is to do with the test suite - it rimraf's the entire node_modules dir, npm installs it again and rebuilds the types from scratch. This shouldn't happen during watching.

I see, makes sense!

Do you need this pushed to a next branch so you can install it easier?

I can keep using the branch, no problem.

One thing to test actually - are you getting type checking when you use state.matches? It should be type checking against all valid values.

Yes, on this:

const appMachine = createMachine<Context, Event, State, 'app'>({
  // ...
  states: {
    idle: {
    // ...
    },
    playing: {
    // ...
    },
  },
});

const service = interpret(appMachine, { /* ... */ });
service.start();
service.state.matches('dummy');

I get this:

Argument of type '"dummy"' is not assignable to parameter of type '"idle" | "playing"'. [2345]

@mattpocock
Copy link
Owner

Alright, great. I'll close this issue and look to roll this out this week.

@mattpocock
Copy link
Owner

@bard I'm actually going to re-open this. I think we can do better. We may even be able to infer typestates without you having to provide them.

@bard
Copy link
Author

bard commented Sep 18, 2020

Hmm, wondering how that would look like. What would you infer them from?

@mattpocock
Copy link
Owner

mattpocock commented Sep 18, 2020

Let's imagine that a user has declared all their assign functions in the second parameter of Machine:

type Context = { hasBeenChanged: 'no' | 'yes' };

const machine = Machine<Context>(
  {
    initial: 'red',
    context: {
      hasBeenChanged: 'no',
    },
    states: {
      red: {
        after: {
          2000: { actions: ['changeContext'], target: 'green' },
        },
      },
      green: {},
    },
  },
  {
    actions: {
      changeContext: assign(() => {
        return { hasBeenChanged: 'yes' };
      }),
    },
  },
);

If we generate a TS graph of the config, we can introspect what each assign action returns. So we know:

  1. What the context shape is in green
  2. What shape the context will be when it transitions to red

This only works, though, if you declare your assign actions in the second parameter of your Machine declaration. But it will work, because we have all the info. Instant typestates, without having to write 'em out.

@bard
Copy link
Author

bard commented Sep 18, 2020

Two thoughts:

  1. Would it work also when you are changing the context based on the previous context?
  {
    actions: {
      changeContext: assign((context, event) => {
        return { ...context, counter: context.counter + event.incrementBy };
      }),
    },
  },

I'd think that the type of context inside changeContext cannot at the same time be known in advance and inferred from the return value.

  1. Even outside of XState, I usually don't mind writing discriminated object unions, because that helps me reason about the possible states of the system away from implementation noise. So maybe I'm just not the ideal "customer" for the feature. :)

@mwarger
Copy link
Contributor

mwarger commented Sep 18, 2020

@bard I'm actually going to re-open this. I think we can do better. We may even be able to infer typestates without you having to provide them.

This only works, though, if you declare your assign actions in the second parameter of your Machine declaration. But it will work because we have all the info. Instant typestates, without having to write 'em out.

Would we be able to provide (or merge in) external typestates in cases where we wanted to provide actions outside of the machine declaration (for example, in the second parameter of useMachine)?

@mattpocock
Copy link
Owner

@Andarist could I get your opinion on this thread? I'm swaying towards the idea that we should support typestates passed in as a generic from the user, but also look to infer them for other users down the line.

@Andarist
Copy link
Collaborator

I think that we shouldn't support custom typestates because it's an unsafe feature and the whole goal of this project is to provide type safety. Or rather - we should not allow them unless one makes a compelling case that they are way better over what we can provide out of the box.

That being said - computing typestates won't happen over night here so as a stopgap we can allow them for the time being but with a plan to remove the support for them when we reach the point of being able to compute them ourselves.

@karneges
Copy link

karneges commented Nov 24, 2020

Hello everyone.I was come across with this problem.But i very want use typescript on 100% .I created small solution,this may look like crutch,but it is very type save
https://codesandbox.io/s/vigilant-sid-n1gdh

@Andarist
Copy link
Collaborator

I don't think this can ever be truly type-safe without a compiler backing this up. It seems nearly impossible to validate at type-level that in a certain state you 100% deal with a particular typestate. To do that you need to analyze the graph.

@karneges
Copy link

Yes you are right. But my solution might help with this. If you use my utility functions, you will get restrictions. Typescript won't give you the wrong context or invalid event if you're honest)
This is a partially manual solution) .But it's help us to avoid,most bugs in compiler stage

@fischor
Copy link

fischor commented Dec 16, 2020

@mattpocock Does #28 bring typestates when you explicitly define them like described in the xstate docs? Sry, I couldn't figure out from this discussion what changes it makes.
At the moment (version 0.21.0) I think its not possible to define them, since the generated createMachine is typed like this

  export function createMachine<
    TContext,
    TEvent extends EventObject,
    Id extends keyof RegisteredMachinesMap<TContext, TEvent>
  >(

If so it would be nice if #28 could be merged, since the typestates are helpful even if you need to define them explicitly.

@mattpocock
Copy link
Owner

@fischor Yes, it did. I'm going to look at this again as part of a wider project to improve xstate-codegen's inferred typings. I may end up using the approach described in #28, but the inferences may look a little different than they do today.

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

No branches or pull requests

7 participants