statelyai / xstate-tools

Public monorepo for XState tooling
183 stars 36 forks source link

Bug: Typegen does not output correct types #297

Open EightArmCode opened 1 year ago

EightArmCode commented 1 year ago

Description

This might be an error in my code, but after a couple of sessions trying to log from the machine-extractor/dist/xstate-machine-extractor.cjs.dev.js file, etc, without results, I thought to ask for help. Can you please help me understand why typegen doesn't seem to like my machine?

// machine.ts
import { createMachine, assign } from 'xstate'

import { Context, Events, States } from './types'

export const graphMachine = createMachine(
  {
    id: 'graph',
    // eslint-disable-next-line xstate/no-invalid-state-props
    predictableActionArguments: true,
    tsTypes: {} as import('./machine.typegen').Typegen0,
    initial: States.idle,
    states: {
      [States.idle]: {
        on: {
          LOAD: {
            target: States.loading,
          },
        },
      },
      [States.loading]: {
        invoke: {
          id: 'loading',
          src: 'fetchGraphData',
          onDone: {
            target: States.loaded,
            actions: ['assignGraphData'],
          },
          onError: {
            target: States.error,
            actions: ['assignError'],
          },
        },
      },
      [States.loaded]: {
        on: {
          LOAD: {
            target: States.loading,
            actions: ['resetContext'],
          },
          UNLOAD: {
            target: States.idle,
            actions: ['resetContext'],
          },
        },
      },
      [States.error]: {},
    },
    schema: {
      context: {} as Context,
      services: {} as {
        fetchGraphData: { data: unknown }
      },
      events: {} as Events,
    },
  },
  {
    services: {
      fetchGraphData: async (context: Context): Promise<unknown> =>
        fetch(context.path),
    },
    actions: {
      assignGraphData: assign({
        graphData: (context) => context.graphData,
      }),
      resetContext: assign((context) => ({
        ...context,
        selectedGraph: '',
        graphData: null,
        path: '',
      })),
    },
  }
)
// types.ts
export const States = {
  idle: 'idle',
  loading: 'loading',
  loaded: 'loaded',
  error: 'error',
}

export type STATES = typeof States[keyof typeof States]

export interface Context {
  selectedGraph: string
  graphData: unknown
  path: string
}

export type Events = LOAD | UNLOAD

interface LOAD {
  type: 'LOAD'
  selectedGraph: string
  path: string
}

interface UNLOAD {
  type: 'UNLOAD'
  selectedGraph: null
  path: string
}
// machine.typegen.ts
// This file was automatically generated. Edits will be overwritten

export interface Typegen0 {
  '@@xstate/typegen': true
  internalEvents: {
    'xstate.init': { type: 'xstate.init' }
  }
  invokeSrcNameMap: {}
  missingImplementations: {
    actions: never
    services: never
    guards: never
    delays: never
  }
  eventsCausingActions: {}
  eventsCausingServices: {}
  eventsCausingGuards: {}
  eventsCausingDelays: {}
  matchesStates: undefined
  tags: never
}

Expected result

I expect typegen to infer some types and output them to machine.typegen.ts

Actual result

It appears I just have the intial output.

Reproduction

https://stately.ai/viz/a89b2aa2-92ca-4c7e-b96d-e66e1272480a

Additional context

Node v v14.19.1
"@xstate/cli": "^0.3.3",
"@xstate/react": "^3.0.1",
"@xstate/inspect": "^0.7.0",
"xstate": "^4.33.6"
"eslint-plugin-xstate": "^1.0.2",
with-heart commented 1 year ago

You need to define your machine types in the same file as the machine. Typegen currently only reads the current file for type information.

You'll likely also have problems using variables for state names, etc., at least for the time being.

Andarist commented 1 year ago

Yeah, you can define Events and Context externally but States should get inlined, so:

// machine.ts
import { createMachine, assign } from 'xstate'

import { Context, Events } from './types'

export const graphMachine = createMachine(
  {
    id: 'graph',
    // eslint-disable-next-line xstate/no-invalid-state-props
    predictableActionArguments: true,
    tsTypes: {} as import('./machine.typegen').Typegen0,
    initial: 'idle',
    states: {
      idle: {
        on: {
          LOAD: {
            target: 'loading',
          },
        },
      },
      loading: {
        invoke: {
          id: 'loading',
          src: 'fetchGraphData',
          onDone: {
            target: 'loaded',
            actions: ['assignGraphData'],
          },
          onError: {
            target: 'error',
            actions: ['assignError'],
          },
        },
      },
      loaded: {
        on: {
          LOAD: {
            target: 'loading',
            actions: ['resetContext'],
          },
          UNLOAD: {
            target: 'idle',
            actions: ['resetContext'],
          },
        },
      },
      error: {},
    },
    schema: {
      context: {} as Context,
      services: {} as {
        fetchGraphData: { data: unknown }
      },
      events: {} as Events,
    },
  },
  {
    services: {
      fetchGraphData: async (context: Context): Promise<unknown> =>
        fetch(context.path),
    },
    actions: {
      assignGraphData: assign({
        graphData: (context) => context.graphData,
      }),
      resetContext: assign((context) => ({
        ...context,
        selectedGraph: '',
        graphData: null,
        path: '',
      })),
    },
  }
)
EightArmCode commented 1 year ago

Is there any plan to allow for the use of variables in machine definition? Or a ticket I can follow or watch?

EightArmCode commented 1 year ago

I'm more likely to write my own types until this is more robust, but I'd really like to use typegen!

Andarist commented 1 year ago

Could you tell us more about why you want this feature? What's wrong with plain state names from your point of view?

I'm more likely to write my own types until this is more robust, but I'd really like to use typegen!

Note that it's nearly impossible to achieve the same type-safety when you write your own types.

EightArmCode commented 1 year ago

It seems that I have a misapprehension about how this should be used, as I experienced a similar conflict trying to apply xstate to another repo.

From my POV, it's hard to imagine a world where I have to inline all my variables, but that seems to be what is required by the library. It sounds like you're suggesting that we should write the machine in one single declaration without adding any types. Am I understanding this correctly?

If this is the case, is there an example somewhere of a machine written this way, with types inferred correctly, and typegen emitting the correct declarations?

Andarist commented 1 year ago

From my POV, it's hard to imagine a world where I have to inline all my variables, [...] It sounds like you're suggesting that we should write the machine in one single declaration without adding any types.

Notice that the adjusted code here is still using variables and types. It's just that the structure of the states is "static" and doesn't use any variables for declaring that part of the whole thing.

EightArmCode commented 1 year ago

Surely you can see how variables might be desirable? I can think of a few reasons off the top of my head. Dynamic states seem like they would be better for:

EightArmCode commented 1 year ago

Thank you for the code update. It does generate types correctly when you remove the variable names from the state object. Still, I'd really like to see an example of a repo that makes use of the typegen file in ideal/correct "xstate way". I have been reading about xstate in depth for a while now, and the available examples of this seem to be limited/incomplete.

Andarist commented 1 year ago

Yes, I can imagine some things - but it's still very useful to hear the feedback from the users. I don't want to assume things because then I might be solving the wrong problem.

Testing

Could you expand on that?

Reuse of machines and base states

Reuse of machines is not blocked by this. The reuse of base states gets really complicated with the visual editing of the machine. When you edit a reused part - what do you expect to happen? Should it propagate to other machines that use this base state? I'm not entirely sure if that's always desirable - it feels a little bit magical to me (that might just be me though). Note that editing such a base state could always lead to introducing errors in machines that you are not looking at the moment, but I guess the same is true when you edit them from the code and not from the visual editor. With the visual editor, it's way less obvious though because you look at the whole machine, and not at its slice.

Separation of concerns

I don't understand this argument. Could you expand on that?

Readability

This is very subjective but I think that for most people this [States.idle]: is less readable than idle:.

File size

This is highly related to the reuse argument. I don't quite think that it matters outside of that argument.

EightArmCode commented 1 year ago

Ultimately, any place you wish to reference your states again, you're repeating yourself, or you have to go to the trouble of instantiating the machine.

Testing Perhaps you want to trigger certain UI interactions and test the machine response. The only way to do that is to duplicate code. Now if you consider that part of the machine has been reused multiple times, this duplication becomes quite repetitive and problematic.

Reuse of base machines It is impossible to reuse states given this paradigm. I also learned on a previous issue posted here that we cannot pass an object to createMachine. This feels very limiting to me. Has this changed in the last few months?

Consider this - Since one of the natural enemies of the FSM is "state explosion" the cleverest approach seems to be keeping the machines quite small. In this case, there would be some overlap in machine configuration/style/implementation, and an obvious benefit from reuse.

I work for a big company where several teams work on different parts of the same codebase. This can get really messy if conventions are not created/enforced. In my mind this can lead to much greater instability than dealing with the side effects of extending an object. Thus being able to abstract layers becomes a big focus of adding tooling to the repo.

It's not clear to me how this conflicts with the visual editor, since underneath determining the final machine behavior should be consistent with the IDE. Just making a guess, but I would expect the override with the highest precedence would be the final outcome of a (dynamically?) modified state.

Separation of concerns Personally, I would prefer to separate the declaration of states from the instantiation of the machine, because logically they're different entities. The state describes the application, the machine instantiation describes its behavior.

Readability You're right that it is subjective, but I would posit seeing States.idle is more visual context than idle