Bug: typegen causes error on valid `sendParent` call with custom `TSentEvent`

with-heart commented 2 years ago

Description

When using sendParent with a custom TSentEvent in a machine using typegen, an error is reported on the action key. From what I can tell, this happens in two cases:

eventsCausingActions entry is xstate.init (entry on root or initial state)
machine's schema.events type includes more than one EventObject

Expected result

sendParent with a custom TSentEvent is typesafe and causes no type errors.

Actual result

sendParent with a custom TSentEvent causes type errors.

Reproduction

https://codesandbox.io/s/strange-tu-ro81mv?file=/src/index.ts

Additional context

The types in index.typegen.ts are copied from local.

If you check without-typegen.ts, you can see that all of these sendParent calls work correctly when not using typegen types.

with-heart commented 2 years ago

Ahhh after a little more digging, it's because TEvent is narrowed by the generated types to the eventsCausingActions entry types. As it should be.

The calls in without-typegen.ts are only working because I'm not accessing event in a callback. Passing the entire event object as the 2nd generic param is too wide.

But then that means to use TSentEvent, we need to manually adjust the 2nd generic param anytime the eventsCausingActions entry changes (because the action is called by another source). It also means that when the entry is xstate.init, we'd need to manually supply that type which seems weird right?

Possibly related to or a duplicate of #2994

with-heart commented 2 years ago

Random thoughts bouncing around in my head rn:

Typegen detects presence of TSentEvent param and generates+replaces 2nd arg with correctly-narrowed event type, so we don't have to manually widen/narrow it depending on how it's used in the machine. Probably not a good path to go down since currently typegen doesn't touch anything in the user's code other than the tsTypes line
Typegen creates a graph from all machines and their parent/child relationships in order to generate TSentEvent from a machine's parents and provide that type to the machine via tsTypes without needing to use generics at all. Provides type safety without needing to modify the user's code, but that's a lot of complexity and could also lead to TSentEvent containing a ton of events that the machine doesn't need to care about
Would it be possible to add a sendParent overload with a single TSentEvent generic param while still that still receives TContext and TEvent from the machine config somehow?

Andarist commented 2 years ago

I've looked through all of the examples and I think that essentially all of them are the same (from our PoV).

It doesn't work because the eventsCausingActions is narrowing the "input" event type to a subset of all event types handled by your machines (like you have mentioned). This is an assignability problem, we can't assign an action object that "holds" multiple events to something that expects a single event. We can't do this because those action objects are covariant there, so this doesn't work in the same way as this doesn't work:

const ev: { type: 'sendMessage' } = {} as { type: 'sendMessage' } | { type: 'sendFile' }

The only chance for us to fix this is to make those actions treated covariantly, we'd have to convince TypeScript that those actions are just functions. In such a case this would work because this would "flip" assignability:

const acceptEv: (ev: { type: 'sendMessage' }) => void = {} as (ev: { type: 'sendMessage' } | { type: 'sendFile' }) => void // OK

This would definitely require some hacks to be done but, in theory, it would be possible and I would like to experiment with this later on.

But then that means to use TSentEvent, we need to manually adjust the 2nd generic param anytime the eventsCausingActions entry changes (because the action is called by another source).

Yes, this is a problem with an action like sendParent. This is mainly the problem because you provide generics explicitly (instead of them being inferred) and I suppose that you want to do this to use the 3rd one, the one specific to sendParent. Unfortunately, in TypeScript we can't selectively supply generics and have the rest be inferred. Note that this isn't type-safe anyway because you can't enforce if a particular machine will get used within another machine that will actually be capable of handling your ParentEvent. So it's more like a type assertion here anyway. For that reason, I'd like to rethink the sendParent API and, in general, cross-actor communication from the type safety perspective.

It also means that when the entry is xstate.init, we'd need to manually supply that type which seems weird right?

Yes, I agree. It feels weird. It's basically caused by what I've mentioned above. I would recommend you skip using those generics. I understand that even though sendParent is not type-safe you might still want to "validate" the TParentType somehow. I would introduce toParentEvent and use it like this: https://codesandbox.io/s/falling-platform-cwo9uy?file=/src/index.ts

You could also abstract this away and maybe even have smth like createSendParent:

function createSendParent<TParentEvent extends { type: string }>() {
  return function <TContext, TEvent extends { type: string }>(
    event: Parameters<typeof sendParent<TContext, TEvent, TParentEvent>>[0],
    options?: Parameters<typeof sendParent<TContext, TEvent, TParentEvent>>[1]
  ) {
    return sendParent<TContext, TEvent, TParentEvent>(event, options);
  };
}

And use it like this: TS playground (codesandbox fails to parse instantiation expressions)

Typegen detects presence of TSentEvent param and generates+replaces 2nd arg with correctly-narrowed event type, so we don't have to manually widen/narrow it depending on how it's used in the machine. Probably not a good path to go down since currently typegen doesn't touch anything in the user's code other than the tsTypes line

It's a possible approach, but a complicated one. I'd liek to explore simpler solutions first.

Typegen creates a graph from all machines and their parent/child relationships in order to generate TSentEvent from a machine's parents and provide that type to the machine via tsTypes without needing to use generics at all. Provides type safety without needing to modify the user's code, but that's a lot of complexity and could also lead to TSentEvent containing a ton of events that the machine doesn't need to care about

Creating a graph like this was on my mind already. This wouldn't work across projects though as for this to work we need to know all of the machines within the system. While you start considering being able to share machines through npm packages etc this starts to get a little bit messy. A graph like this would still be useful for visualization purposes though and it's something we will probably do at some point. Also note that I'd like to fix the type-safe of all cross-actor communication, and not just the sendParent.

Would it be possible to add a sendParent overload with a single TSentEvent generic param while still that still receives TContext and TEvent from the machine config somehow?

I guess that I sort of created this above with createSendParent. I wouldn't like to make this an official util though. At least not before we take a stab at reimagining the type safety of cross-actor communication.

Grimones commented 1 year ago

Hi all If anyone stumbles upon this not working with xstate@4.37.0 that's becaue of this change

https://github.com/statelyai/xstate/pull/3694/files#diff-c5830f238a742a03618be5af0725793f3f15a959ba8d49cac691b575153fb29cL301

I've found a fix. Not sure how correct it is, but hey, it works :)

export function createSendParent<TParentEvent extends EventObject = AnyEventObject>() {
  return function <TContext, TEvent extends EventObject, TSentEvent extends EventObject = AnyEventObject>(
    event: Event<TParentEvent> | SendExpr<TContext, TEvent, TParentEvent>,
    options?: Parameters<typeof sendParent<TContext, TEvent, TParentEvent>>[1],
  ) {
    return sendParent<TContext, TEvent, TSentEvent>(event, options);
  };
}

The change is in event parameter passing TParentEvent to the types

davidkpiano commented 1 year ago

cc. @Andarist ^ Looks like a good potential fix

Andarist commented 1 year ago

Ye, this is a way to "bind" sendParent with a specific type of event meant for a parent. It's not exactly something that we could incorporate into our types though and introducing a helper might not be with it if we are exploring sendParent removal in v5. It's not exactly that this helper makes it totally safe - it just trusts your declaration but the actual parent might not accept those configured events or might even be incompatible with the declared type.

statelyai / xstate