mattpocock / xstate-codegen

A codegen tool for 100% TS type-safety in XState
MIT License
245 stars 12 forks source link

Typestates? #27

Open bard opened 4 years ago

bard commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

mattpocock commented 4 years ago

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?

bard commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

bard commented 4 years ago

@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 commented 4 years ago

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

mattpocock commented 4 years ago

@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 commented 4 years ago

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

bard commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

mattpocock commented 4 years ago

@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 commented 4 years ago

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

mattpocock commented 4 years ago

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 commented 4 years ago

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.

2) 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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

@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 commented 3 years ago

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