Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[V5] [Bug]: typescript errors when spawning actors during context initialization #4566

Closed
bhvngt opened this issue Dec 7, 2023 · 8 comments · Fixed by #4568
Closed

[V5] [Bug]: typescript errors when spawning actors during context initialization #4566

bhvngt opened this issue Dec 7, 2023 · 8 comments · Fixed by #4568
Assignees

Comments

@bhvngt
Copy link

bhvngt commented Dec 7, 2023

Bug or feature request?

Bug

Description:

When I am trying to spawn an actor during context initialisation, typescript throws an incorrect error linking one of the invokable actors with spawned actor.

(Bug) Expected result:

It should not throw any typescript error

(Bug) Actual result:

image

Link to reproduction:

https://stackblitz.com/edit/xstate-v5-bug?file=package.json,src%2Fcounter.ts

Additional context

I could not find any Bug template for filing bugs. Hence I have filed this using a blank template.

@Andarist
Copy link
Member

Andarist commented Dec 7, 2023

There are some issues with this code. if you setup concrete actor types then you shouldn't use Child directly with spawn. Instead, you should reference it with a string (spawn('Child')). So you should also actually setup this one since right now you are only setting up two.

That being said - even with that fix it doesn't work with TS 5.0. Our types rely currently on a pattern enabled by TS 5.1 ( microsoft/TypeScript#53098 ). I prepared a fix for this here so soon it should also work with TS 5.0

You can see the working example here: TS playground

I also wouldn't recommend doing this:

const incBy = assign(({ context }, params: number) => {
  return { count: context.count + params };
}) as AssignAction<any, any, any, any, any>;

While you declared the params type within this declaration you essentially forget it by casting to AssignAction<any, any, any, any, any>. This might actually lead to problems because now params won't be type-checked correctly and they won't error when you try to use the incorrect ones: TS playground

To fix this it's best to declare your assign "inline", within setup itself. This enables the types to flow from context into this assign. You can see this in practice here: TS playground

@bhvngt
Copy link
Author

bhvngt commented Dec 7, 2023

Thanks @Andarist as always for your detailed explanation. In the hindsight it make sense to treat spawned actors the same way as invoked actors.

That being said - even with that fix it doesn't work with TS 5.0. Our types rely currently on a pattern enabled by TS 5.1 ( microsoft/TypeScript#53098 ). I prepared a fix for this #4568 so soon it should also work with TS 5.0

My reproduction uses TS 5.3. I can see that the error goes away in the TS playground when 5.3 is chosen. Not sure, why it is showing the error in stackblitz in spite of the latest typescript version. I get the same problem in my local setup too.

To fix this it's best to declare your assign "inline", within setup itself. This enables the types to flow from context into this assign. You can see this in practice here: TS playground

Got it. I do have few common use cases where I would prefer to use shared actions such as storeError, resetError etc. I understand that I will loose the type safety for those actions. I guess as long as they are few and clear, it should be alright!!

@Andarist
Copy link
Member

Andarist commented Dec 7, 2023

Not sure, why it is showing the error in stackblitz in spite of the latest typescript version.

My bet is that it doesn't really use the TS version installed by your project there but rather whatever they have bundled with their editor.

I get the same problem in my local setup too.

Could you try using "Select TypeScript version..." from your command palette (I'm assuming that you are using VS Code) and use the workspace version?

@bhvngt
Copy link
Author

bhvngt commented Dec 7, 2023

My bet is that it doesn't really use the TS version installed by your project there but rather whatever they have bundled with their editor.

I guess you are right.

Could you try using "Select TypeScript version..." from your command palette (I'm assuming that you are using VS Code) and use the workspace version?

I am using Jetbrains. I found a small typescript issue with my code. On fixing that the error in my local setup is gone.

Thanks @Andarist.

@karlanke
Copy link

I'm still seeing this with typescript 5.3.x, I think. I'm grabbing a user record, and want to spawn a dynamic number of machines based on the user's access level (and, incidentally, avoid having a giant list of if (accessToMachine4) spawn("machine4") blocks in code - my idea is to have a single place listing the available machines by name. Alternately, I could be misunderstanding something!

image image image

@davidkpiano
Copy link
Member

@karlanke Currently, if you specify known actors in setup(…), you can only reference those actors in spawn.

Maybe we can find some way to loosen this constraint (cc. @Andarist)

@karlanke
Copy link

Is there an easy way to do that dynamically? I took a quick stab at it and it didn't immediately accept a

actors: {
userActor,
...dynamicActors
}

but admittedly I didn't spend a lot of time on it.

As a workaround, for anyone who finds this, that constraint seems to disappear if you provider input to the actor being spawned.

I don't think this is a huge useability problem, since the actor can still be typed fully, but glad to know what the cause is.

@davidkpiano
Copy link
Member

@karlanke Let's open a separate issue for this, so it doesn't get lost 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants