xstate-codegen icon indicating copy to clipboard operation
xstate-codegen copied to clipboard

Typestates?

Open bard opened this issue 5 years ago • 34 comments

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')) { ... }?

bard avatar Sep 14 '20 11:09 bard

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 avatar Sep 14 '20 12:09 mattpocock

@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.

mattpocock avatar Sep 14 '20 12:09 mattpocock

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.

bard avatar Sep 14 '20 12:09 bard

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 avatar Sep 14 '20 12:09 mattpocock

@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 };

mattpocock avatar Sep 14 '20 12:09 mattpocock

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?

bard avatar Sep 14 '20 12:09 bard

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 👍

mattpocock avatar Sep 14 '20 12:09 mattpocock

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.

davidkpiano avatar Sep 14 '20 12:09 davidkpiano

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

mattpocock avatar Sep 14 '20 13:09 mattpocock

Got a POC working here:

https://github.com/mattpocock/xstate-codegen/pull/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?

mattpocock avatar Sep 14 '20 13:09 mattpocock

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;
}
*/
  }

bard avatar Sep 14 '20 13:09 bard

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.

mattpocock avatar Sep 14 '20 14:09 mattpocock

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

davidkpiano avatar Sep 14 '20 14:09 davidkpiano

@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 avatar Sep 14 '20 15:09 bard

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

bard avatar Sep 14 '20 15:09 bard

@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.

mattpocock avatar Sep 14 '20 16:09 mattpocock

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

bard avatar Sep 14 '20 17:09 bard

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.

bard avatar Sep 15 '20 08:09 bard

@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 avatar Sep 15 '20 08:09 mattpocock

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

mattpocock avatar Sep 15 '20 09:09 mattpocock

@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]

bard avatar Sep 15 '20 10:09 bard

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

mattpocock avatar Sep 15 '20 11:09 mattpocock

@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.

mattpocock avatar Sep 18 '20 15:09 mattpocock

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

bard avatar Sep 18 '20 15:09 bard

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.

mattpocock avatar Sep 18 '20 16:09 mattpocock

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. :)

bard avatar Sep 18 '20 16:09 bard

@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)?

mwarger avatar Sep 18 '20 17:09 mwarger

@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.

mattpocock avatar Sep 21 '20 13:09 mattpocock

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.

Andarist avatar Sep 22 '20 17:09 Andarist

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

karneges avatar Nov 24 '20 22:11 karneges