[core] Add actions to model

[davidkpiano]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 2158468

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[Andarist]

"Base" doesn't tell us much - what do you think about changing that to "Inline"? I'm not entirely sure if that was the intention but it seems to hold right now. And there might be some benefit in having InlineAction and ImplementationAction split anyway.

[davidkpiano]

The intention is for there to be an action object without exec; I typically use Base* as a type name to signify a type that other types build on. But this isn't for a specific type of action; it's for all actions.

[Andarist]

This would still, potentially, leave BaseActionObject untouched.

[davidkpiano]

Ah I thought you were talking about BaseActionObject - maybe naming this PlainActions would be better

[Andarist]

ActionObject has an exec property - it seems to me that this wouldn't be desired here, as this type is meant to only represent "action descriptions". What do you think about changing the constraint to the BaseActionObject? Where maybe the name of this type could be improved in the process (not sure if using BaseActionObject would fit this location here best)

[davidkpiano]

BaseActionObject makes sense as the base for other action objects; let's keep the name for now. It's not meant to be used externally anyway.

[Andarist]

What about changing the extends constraint here?

[davidkpiano]

To BaseActionObject? Works for me

[Andarist]

This condition is slightly indirect. It maps all other properties to never and then checks if T is a subtype of that, where it won't be if any key has been successfully remapped. The truthy branch is chosen when we end up with T extends {}.

A more straightforward approach to this problem could like this:

type ExtractSimple<T> = T extends any
  ? Exclude<keyof T, "type"> extends never
    ? T
    : never
  : never;

/**
 * Extracts action objects that have no extra properties.
 */
type SimpleActionsFrom<T extends BaseActionObject> = ActionObject<
  any,
  any
> extends T
  ? T // If actions are unspecified, all action types are allowed (unsafe)
  : ExtractSimple<T>;

This seems to test if the union member has only the type property more explicitly.

Note that we already have a slight variation of this in this very file:

xstate/packages/core/src/types.ts

Lines 261 to 265 in 58fd61d

	type ExtractSimple<A> = A extends any
	? {} extends ExcludeType<A>
	? A
	: never
	: never;

[davidkpiano]

Sure, feel free to refactor this if the outcome is the same.

[devanshj]

(Just for the sake of bike-shedding & perhaps micro-blogging :P)

The most principally correct way of writing ExtractSimple is this...

type ExtractSimple<T> = T extends any ? { type: Prop<T, "type"> } extends T ? T : never : never

// for posterity
type Prop<T, K> = K extends keyof T ? T[K] : never

What we ultimately want to check is -- is { type: action.type } assignable to action and that's exactly and literally what we're doing when we write { type: Prop<T, "type"> } extends T ? T : never.

All other ways of writing it are, if I may, incorrect. Even if they produce same results. And fun fact -- they don't. For example in Matuesz's ExtractSimple what he's doing is "check if there are no keys other than type" which is not what we want to check -- for ExtractSimple<{ type: "foo", bar?: number }> his version would produce never and my version would produce { type: "foo", bar?: number } and mine is correct (unless the original requirement is not about assignability)

The reason I often champion to be "principally correct" rather than say "utilitarian correct" is that you don't have to think about edge cases. For example I didn't write my version thinking about the optional edge case, I just wrote it the way it was supposed to be written and then I just happen to realize this downside in Matuesz's version.

But why do I say "Even if they produce same results"? For being future proof. What if tomorrow typescript introduces a modifier say "unqiue" so that keyof { unqiue type: "foo" } is "type" but { type: "foo" } is not assignable to { unqiue type: "foo" } (unqiue essentially meaning that property value can't and shouldn't be produced alone) -- in that case my version would still work and some other version might fail.

Another good take away is that we aren't used to leveraging the fact that extends checks assignability and come up with really obscure ways to check it, like Steve was doing and I suggested a refactored and even the versions here are somewhat indirections.

My sincere apologies to use this space as my blog (high time I make one) and please ignore this comment -- use any version it doesn't matter much :P

[Andarist]

I actually thought about the optional properties case myself - just didn't figure out how to support it and put it aside as potential future improvement.

Thanks for providing the variant handling this!

[devanshj]

Ah gotcha. You're welcome!

[davidkpiano]

@Andarist What are the remaining tasks here?

[Andarist]

Just don't forget to add a changeset! 😉

[devanshj]

Also the major concern I have about these action factories is that they only partially enable the user to provide the schema, the schema also should include implementation types. So I'd suggest that we first concretely think about the schema and address it's current shortcomings before adding features to createModel and publishing them.

[davidkpiano]

Also the major concern I have about these action factories is that they only partially enable the user to provide the schema, the schema also should include implementation types. So I'd suggest that we first concretely think about the schema and address it's current shortcomings before adding features to createModel and publishing them.

Can you show an example of the shortcoming here?

[devanshj]

Sure. Take this example.

// Person A wrote the machine
let loginMachine = createMachine({
  schema: {
    context: createSchema<{ user: null } | { user: User }>(),
    events: createSchema<{ type: "LOGIN", credentials: Credentials }>(),
    actions: createSchema<{ type: "assignUser" }>,
  },
  context: { user: null },
  initial: "idle",
  states: {
    idle: { on: { LOGIN: "loggingIn" } },
    loggingIn: {
      invoke: {
        src: "logInWithCredentials", // let's assume txstate knows this type via schema
        onDone: "loggedIn"
      }
    },
    loggedIn: {
      entry: "assignUser"
    }
  }
})

// Person B is using it
useMachine(loginMachine, {
  logInWithCredentials: () => /* whatever */,
  assignUser: assign({ user: (_, e) => e.data })
})

All good now let's say that the Person A makes the following change...

- initial: "idle",
+ initial: "loggedIn"

Now Person B would get a compile error that property data does not exist on e, e is { type: "xstate.init" } | { type: "idk the type :P", data: User } but it's the Person A's fault, we need to show the error while Person A is authoring the machine. This could be fixed with my version of the schema.

  schema: {
    context: t<{ user: null } | { user: User }>(),
    events: { LOGIN: t<{ credentials: Credentials }>() },
    actions: { assignUser: t<(c: never, e: { data: User }) => Assign<{ user: User }>>() },
    behaviors: { logInWithCredentials: t<(c: never, e: { credentials: Credentials }) => PromiseBehavior<User> }
  },

Now when Person A makes that change they'd get an error like so...

      entry: "assignUser"
//           ~~~~~~~~~~~~
// "assignUser" can't be assigned to "Error: `assignUser`'s event requirements are not matched"
// (we'll make the error as specific as possible just an example here)

So the schema indeed needs to provide implementation types for a good experience. Action factories will only merely provide the names and shape of the actions but not their requirements.

And I'm aware the recommended way would be to write { target: "loggedIn", actions: "assignUser" } but that's not the point :P

This example may look like a "nice-to-have" feature but for some things I have in my mind it's crucial txstate knows what the implementations are actually doing (for example here the Assign<{ user: User }> part is important to derive typestates), just their names aren't enough.

[davidkpiano]

@Andarist Note that in model.test.ts there are some type errors; I narrowed the issue to typeof userModel not being assignable to Model<any, any, any, any> because the events are now never:

Can you please take a look? We should fix these before merging.

[Andarist]

@davidkpiano sure, I have not noticed them when I was working on this branch - nor CI has failed 🤔 Gonna give it a spin first time in the morning tomorrow

[davidkpiano]

@davidkpiano sure, I have not noticed them when I was working on this branch - nor CI has failed 🤔 Gonna give it a spin first time in the morning tomorrow

Strangely enough, CI will pass even with these errors present (they show in VS Code), maybe this has something to do with strict mode not being on?

I fear that these errors will appear where strict: true in other codebases.

[devanshj]

Nitpick: the correct way to do Foo<any, any> is to actually Foo<FirstParamConstraint, SecondParamConstraint>¹ like I do it here. My suspicion is bringing any to picture is always dubious², when merging txstate I'll fix this everywhere :P Would also bring the naming convention, it's based of the fact that the implicit constraint of a type parameter is unknown hence when something is used as a constraint it needs to be UnknownSomething.

And not only that it's also important to test if the concrete version is assignable to the unknown version (like I test if Behavior<never, "foo"> which is basically PromiseBehavior<"foo"> is assignable to UnknownBehavior or not), hence one has to get the variance of the type parameters right. I first got it wrong for Behavior so I changed all methods from x: (a: A) => B to x(a: A): B because the latter is bivariant which is a less strict. (Which admittedly was a quickfix I'll see if I can manage to get it work keeping the variance strict)

Saying in case it helps resolving the problem here I haven't taken a look at it tho :P

¹ By doing so you'll have constraints rolling down like so...

type UnknownEvent = { type: string }
type Foo<E extends UnknownEvent> = E["type"]
type UnknownFoo = Foo<UnknownEvent>
// UnknownFoo will be string
type AnyFoo = Foo<any>
// AnyFoo will be any (afk didn't confirm)

² For example in type T = any extends "foo" ? "a" : "b", T is "a" | "b" which you most probably don't want. You likely want unknown instead of any which would give "b" instead (afk haven't double checked).

[davidkpiano]

@Andarist @devanshj Can you please double check this?

My VS Code was showing errors (though mysteriously, CI was passing) because Cast<never, EventObject> resulted in never, when I would expect it to result in EventObject. This is because never extends EventObject (which still confuses me) but I added a check to make sure this doesn't happen.

[devanshj]

The short answer is I'd very strongly suggest to not change Cast because that's not the problem here and it's behaving perfectly fine. I'd suggest you to check for never at the place of issue and then swap it with EventObject.

But the fact that you need a check like this is weird can you tell me the original problem? Where exactly Cast<never, EventObject> being never was a problem? I'll clone the repo. Edit - okay found it looking into it.

I'm sure you know some of this but we use Cast in places like...

type Foo<T extends string> = {}
type Bar<T> = Foo<Cast<T, string>>
// Foo<T> doesn't match the constraint

But if you have...

type Foo<T extends string> = {}
type Bar<T extends never> = Foo<T>
// no problemo no point of using Cast and even if you use it Cast<never, string> will be never

So the fact you need your version of Cast itself is fishy. (also writing a longer explanation for your doubt)

[davidkpiano]

For reference, the problem happens here: #2266 (comment)

[davidkpiano]

@devanshj The reason we need Cast here is because of this:

Type 'TComputedEvent' does not satisfy the constraint 'EventObject'.ts(2344)

Even though we know that TComputedEvent will always be an EventObject, TS doesn't seem to know this.

[devanshj]

Yep I understand the original requirement of cast, that's valid. But wanting Cast<never, EventObject> to be EventObject (which is not "casting" behavior as never already suffices) is invalid. As I suspected the problem is something else. It's this...

TypeScript is picking the second overload, but why? Let's remove the second overload and let's keep only the model overload then this happens...

What typescript is saying is typeof userModel does not extend Model<any, any, any, any>, we can even check it...

Which basically means userModel can't be assigned to Model<any, any, any, any>...

And why is that? Because...

So perhaps what you were trying to do is make the never to be EventObject so that it works...

But that could be the incorrect way of solving it. Here are two possible reasons of the original problem -

1. createModel definition is bad, it should never return Model<X, never>
2. Model type definition is bad, Model<X, never> should extend Model<any, any> (or UnknownModel to be precise)

Now if the reason is former then your fix is correct to never return Model<X, never> but I'm suspicious the problem is latter. Why? I had a very similar problem with Behavior...

createBehaviorFromPromise returns Behavior<never, T> but it wasn't assignable to UnknownBehavior. Here too there could be the same two reasons - either createBehaviorFromPromise is bad or Behavior is bad. It was very apparent from the definition of Behavior that a promise behavior should be Behavior<never, T> - so createBehaviorFromPromise was right and the problem was with Behavior. So I relaxed all the call signatures in Behavior so that the it's less strict and assignability works (the changes are in the same commit you can take a quicklook). I could have made promise behavior to be Behavior<UnknownEvent, T> to make the assignability work but that would incorrect because a promise behavior takes no events (except for the start and stop signals which aren't passed in the first type parameter)

Now in our case even if let's say the reason is latter, xstate is way too filled with "type-debt" to even fix it (I did attempt to change the variance of things all the way down but no luck). So I'd say let's just pretend the problem is former and never produce Model<X, never>.

But your definition of cast will make cast no longer cast (I'll explain later why never extends X holds), cast isn't the problem here and changing it will result in bigger problems so that's an absolute no-go. What you should do is this....

IsNever<TComputedEvent> extends true ? EventObject : Cast<TComputedEvent, EventObject>

I hope it helps! And sorry/welcome for the long explanation :P (Tho I'm yet to explain why never extends X holds, will do xD)

[davidkpiano]

@devanshj I'm pretty sure we want the events for Model to never be never, because a model without events specified should result in a machine that can accept an EventObject.

This commit implements your suggestion: 458abd5

[devanshj]

because a model without events specified should result in a machine that can accept an EventObject

Why would you say that? A machine based on Model<{}, { type: "FOO" } | { type: "BAR" }> only takes events that are assignable to { type: "FOO" } | { type: "BAR" }. Then in the same way a machine based on Model<{}, never> should take events assignable to never which basically means it should take no events.
Ah wait gotcha, you mean that if createModel has no event creators then we should fallback to taking any events ie { type: string }, well if that's a conscious choice then it's okay.

But I still think Model<{ foo: string }, never> should extend Model<unknown, { type: string }> the root problem is that Action<{ foo: string }, never> is not assignable to UnknownAction which is Action<unknown, UnknownEvent> which makes me think UnknownAction should be Action<never, never> instead. So maybe instead of writing Model<any, any> one should write Model<any, never> idk. I'm not sure what's the "principally correct" way to solve it I haven't thought deeply about this.

But all this aside given you're consciously making that choice then it's okay to do what you're doing with CastIfNever. Though to nitpick I'd suggest you to inline the never check and remove the CastIfNever (you can keep IsNever). Having that type would encourage it's usage where it not might be apt like a future person would go "Hmm Cast is not working, I see there is a CastIfNever" *slaps that* "Ha now it's working cool" and calls it a day xD instead of looking into the problem. But feel free to take your call, if you'd like to keep CastIfNever do that.

[Andarist]

Ok, I think I have finally digested this thread.

Would it be correct to say that David's problem was caused by the variance issues? Model["assign"] is a function so its parameter types are contravariant. This should not result in an error under strictFunctionTypes: false which we currently have implicitly configured - we plan to change this and enable strict mode in the whole repo but this has not been done yet. I don't get why both you and David have seen this error and why I can't see it on my machine, nor CI fails. That being said - it's a good thing that this has been caught now but I don't understand why it was caught 😬

I also quite don't understand the UnknownBehavior fix - I've tried to replicate that here and it doesn't type check (which makes sense to me with strictFunctionTypes: true). How your Behavior/UnknownBehavior is different from this?

---

In general, I agree with @devanshj - this statement seems like an off choice:

model without events specified should result in a machine that can accept an EventObject.

If the whole point of types is to make our apps safer then we shouldn't produce unsafe variants by default. If the consumer doesn't want to type something, they are in a rush or having trouble doing so, then they should explicitly annotate stuff as being unsafe (with any). This IMHO should always be a conscious decision made by a user and not something that we promote by effectively making that a default that can be produced unintentionally.

[davidkpiano]

If the whole point of types is to make our apps safer then we shouldn't produce unsafe variants by default. If the consumer doesn't want to type something, they are in a rush or having trouble doing so, then they should explicitly annotate stuff as being unsafe (with any). This IMHO should always be a conscious decision made by a user and not something that we promote by effectively making that a default that can be produced unintentionally.

Okay, that makes sense. I'll make the appropriate changes.

[devanshj]

Would it be correct to say that David's problem was caused by the variance issues? Model["assign"] is a function so its parameter types are contravariant.

Right. Idr if it was the assign that caused the issue or not but you're probably right that it was assign.

I don't understand why it was caught 😬

Maybe our vscode was buggy and didn't pick the tsconfig? xD

I've tried to replicate that here

Not sure by "replicate" you meant replicate the bug or replicate the fix. If you meant the latter then nope xD Look closely the call signatures are different from Behavior, here's how you fix it (type also works but using interface just to be on a safer side). I tried applying this fix that day, either I applied it incorrectly or your repro is too minimal.

So to go forward try applying this fix yourself and see if it works, if not try producing an isolated repro so that I can look into it and if none of this works then I might hop into the actual code :P

And this for my standards still will be a "quickfix" as I said here -

Which admittedly was a quickfix I'll see if I can manage to get it work keeping the variance strict

The fact that you have to make the parameters bivariant to make it work (which might also create a gap for bugs) tells me something is off here, probably in the definition Behavior. I might have to write the implementation myself to see what definition organically emerges - if it's the same then that would be mean the type system is at odds with this kind of stuff and making the parameters bivariant will be only choice. Or wait maybe UnknownBehavior should be something other than Behavior<UnknownEvent, unknown> not sure haven't dealt with it yet.

[Andarist]

So I guess that the createMachine<typeof userModel> should work as well but fixing that can wait for @devanshj's feedback because I'm unsure how this should be refactored to allow that in the strict mode.

[davidkpiano]

Right. model.createMachine does not give me type errors.