Support for machine options?

[danielkcz]

Just tried the library for the first time and got surprised that I cannot pass options in the second arg. Any reason for that? That's a kinda crucial part of the XState imo :)

[mattpocock]

I agree, we need to support this - here's the current state of things:

https://www.loom.com/share/9852be4f9d1f455eb76521d63bfe4059

[rjdestigter]

Including chat from https://spectrum.chat/statecharts/general/xstate-codegen-gives-you-100-type-safety-in-xstate~afc9d5d7-86d2-4771-887f-247487dc8a17 for further discussion:

A: Using string identifiers and passing the configuration as a second argument to createMachine is not part of the code gen correct?

B: Not yet, but it should be! We have all the information to allow that. Could you post an issue?

In fact, there are some complications here. Let's say that you declare a state machine, and pass in some of the options into the second argument. How would you want the types when you interpret the machine to behave?

I want the types to be as strong as possible, which means you would likely need to require everything in interpret that you didn't add in the second argument to Machine.

But you want it to be overrideable, so you'd need to also allow options declared in Machine to be optional in interpret. Complex, but do-able (probably)

[danielkcz]

Hi Matt, thanks for your analysis. Machine options are mainly useful if I want to declare some actions/service as built-in into the machine so it can be easily reused as a whole. And on top of that, there might be cases where actions/services are used in multiple occurrences within the machine so inlining it would be rather painful.

As for the approach which to take, if that's possible, it would be really nice if all options are required by default (as you have now), but when specified with machine options it becomes optional for interpreting. That makes the most sense to me so you are still able to override, but you not forced to. It's also great that as a machine consumer you instantly know what you need to implement.

[mattpocock]

@FredyC I had a quick look at this and it's actually trivial to implement. When we introspect the machine we have a list of all the options passed to it, so we can work out which should be required in 'interpret' contexts and which not.

The spec will be as follows:

Options passed to the second argument of Machine will all be optional. Options passed to the 'interpret' function will be all required, except for the ones you declared inline in Machine, which will be optional.

What do you think?

[danielkcz]

Definitely love it, especially if it's trivial to implement 💯

[mattpocock]

@FredyC perfect - I'm on holiday for 10 days after today but I'll make this a priority for when I'm back. I'd also welcome a PR on this.

[rjdestigter]

What do you think?

That's exactly what I was going to reply with :D

[mattpocock]

Fixed by #18, once that PR is complete.

[mattpocock]

@FredyC @rjdestigter Could you take a quick look at this?

https://github.com/mattpocock/xstate-codegen/pull/18/files

If you check it out locally, build the package and run yarn link, you should be able to see the optional types.

[rjdestigter]

I'm getting errors trying to build:

src/extractMachines.ts(184,39): error TS2345: Argument of type 'NodePath<Statement> | null' is not assignable to parameter of type 'NodePath<Statement>'.
  Type 'null' is not assignable to type 'NodePath<Statement>'.
src/extractMachines.ts(198,11): error TS2345: Argument of type 'NodePath<Statement> | null' is not assignable to parameter of type 'NodePath<Statement>'.
  Type 'null' is not assignable to type 'NodePath<Statement>'.

[mattpocock]

@rjdestigter Not seeing this locally... Could you try casting the relevant failures as any? This code is going to be removed soon so I feel comfortable doing that.

[rjdestigter]

Yeah, I'm casting ..! to get rid of the null issue. I'm running into a .tsx files being imported though so now I'm looking at if it needs additional babel plugins to support that.

[mattpocock]

Is it freezing at some JSX? You're right, we don't have a plugin for that.

[rjdestigter]

Yeah:

SyntaxError: C:\...\alert\index.tsx: Unexpected token, expected "," (53:20)

  51 |
  52 |   const list = alerts.map((alert, index) => (
> 53 |     <AlertComponent key={index} status={alert.status}>
     |                     ^
  54 |       ...

I'm assuming it's the JSX. However babel-plugin-transform's default is to not preserve the JSX from what I've caught so far.

[mattpocock]

OK, I've added this as a separate issue: #21

Let's continue that chat there

[rjdestigter]

I commented out the JSX to test out this PR. It's not allowing a second argument still for createMachine.

Edit: Hold that thought. I use dashes in ids and src names a lot.

[mattpocock]

@rjdestigter My bad, silly mistake. Try pulling?

[mattpocock]

In fact, I haven't yet fixed the issue. 2 secs.

[rjdestigter]

I tried the branch with a simple machine like this:

const foobar = createMachine<{}, { type: 'FOOBAR' }, 'foobar'>({
  id: 'foobar',
  initial: 'noop',
  states: {
    noop: {
      on: {
        FOOBAR: {
          actions: 'foobaraction'
        }
      }
    }
  }
}, {
  actions: {
    foobaraction: () => {}
  }
})

The compiled version of createMachine however does not allow for passing a second argument:

  export function createMachine<
    TContext,
    TEvent extends EventObject,
    Id extends keyof RegisteredMachinesMap<TContext, TEvent>
  >(
    config: MachineConfig<
      TContext,
      TEvent,
      RegisteredMachinesMap<TContext, TEvent>[Id]['_subState']
    >,
  ): RegisteredMachinesMap<TContext, TEvent>[Id];

Edit: Just saw your comments. Pulling..

[mattpocock]

@rjdestigter OK, here's the latest commit:

https://github.com/mattpocock/xstate-codegen/pull/18/commits/e5438e1573eaa26568d69413651b84fdf369f5e4

This should work

[rjdestigter]

This works for me now:

const foobar = createMachine<{}, { type: 'FOOBAR' | 'JUMP'}, 'foobar'>({
  id: 'foobar',
  initial: 'noop',
  states: {
    noop: {
      on: {
        FOOBAR: {
          target: 'oop',
          actions: 'foobaraction'
        }
      }
    },
    oop: {
      on: {
        JUMP: {
          target: 'noop',
          actions: 'jumpaction'
        }
      }
    }
  }
}, {
  actions: {
    foobaraction: (ctx, evt) => evt.type,
    jumpaction: (ctx, evt) => evt.type
  }
})

(I had hoped that evt.type would've been limited to the events that use the actions but I understand that that could complicate things.)

[mattpocock]

(I had hoped that evt.type would've been limited to the events that use the actions but I understand that that could complicate things.)

It should be! The reason it's not working is that your event is typed slightly incorrectly. It should be:

const foobar = createMachine<{}, { type: 'FOOBAR' } | { type: 'JUMP' }, 'foobar'>({
  id: 'foobar',
  initial: 'noop',
  states: {
    noop: {
      on: {
        FOOBAR: {
          target: 'oop',
          actions: 'foobaraction'
        }
      }
    },
    oop: {
      on: {
        JUMP: {
          target: 'noop',
          actions: 'jumpaction'
        }
      }
    }
  }
}, {
  actions: {
    foobaraction: (ctx, evt) => evt.type,
    jumpaction: (ctx, evt) => evt.type
  }
})

[rjdestigter]

Cool! I commented out the foo machine end re-enabled my big complicated one and it's all compiling now. Unrelated to this PR likely but I did have to change a top-level:

on: {
      REFRESH: "downloading",
    },

to:

on: {
      REFRESH: ".downloading",
    },

(Add period in front of target)

[rjdestigter]

Oh man! It's so nice to be able to remove (evt as TypeName) in my configuration object!

[mattpocock]

Feels so good in those monster machines!

On the above, is there a test case here that needs to be added?

https://github.com/mattpocock/xstate-codegen/blob/master/packages/xstate-compiled/src/__tests__/getTransitionsFromNode.test.ts

[rjdestigter]

Maybe?

    const machine = Machine({
      initial: 'red',
      states: {
        red: {},
        green: {},
      },
    });

    expect(getTransitionsFromNode(machine)).toEqual(['.red', '.green']);
  });

Maybe this test should include targets that also start without the period?

[mattpocock]

@rjdestigter Fixed this here:

https://github.com/mattpocock/xstate-codegen/pull/22

[rjdestigter]

With the PR tested I'd consider this issue resolved

[mattpocock]

Grand, I'll merge and release tomorrow.