-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] Actor types #4036
[v5] Actor types #4036
Conversation
davidkpiano
commented
May 22, 2023
•
edited
Loading
edited
🦋 Changeset detectedLatest commit: 76b3fb7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 76b3fb7:
|
packages/core/test/invoke.test.ts
Outdated
// TODO: investigate why event is DoneInvoke<any> instead of | ||
// DoneInvoke<{result: ...}> | ||
// The type is properly inferred for: | ||
// actions: ({ event }) => event.output.result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nested inference contexts strike again 😬
I think the conditional type that distributes over "non-fixed" (aka not yet set in stone) TActors
is at fault here. I prepared a somewhat minimal repro of this problem: TS playground
Note that the type for that action object is pulled directly from the constraint, if we do this:
interface ActorImpl {
src: string;
- output: any;
+ output: unknown;
}
then we'll see DoneInvokeEvent<unknown>
instead of DoneInvokeEvent<any>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any workaround to this problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convince TS team to merge my microsoft/TypeScript#48838 😆 see how it works just alright in the playground that uses that PR: TS playground
On a serious note, I'd have to spend some time to figure this out for the current TS capabilities (if it's even possible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one still falls back to the constraint - despite the fact that they have callable signatures already. I doubt that this is something that we can control rn without landing the mentioned fix and all I can do is to ping the TS team periodically to give it another look 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That PR of mine mentions that the position of the conditional type can change the inferred result - so I guess I still have to try this out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried moving this conditional type in a variety of ways and couldn't make it work. I think it's best to assume, for now, that it's not possible to improve this and we can revisit this later.
@@ -1187,9 +1191,10 @@ describe('typegen types', () => { | |||
} | |||
}, | |||
{ | |||
// @ts-expect-error | |||
// TODO: determine the exact behavior here and how eventsCausingActors + TActor should interact with each other |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: let's discuss this
@@ -947,7 +951,7 @@ describe('typegen types', () => { | |||
fooActor: fromCallback((_send, onReceive) => { | |||
onReceive((event) => { | |||
((_accept: string) => {})(event.type); | |||
// @x-ts-expect-error TODO: determine how to get parent event type here | |||
// @ts-expect-error TODO: determine how to get parent event type here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: I can try to do that in a follow up PR
// TODO: do not accept machines without all implementations | ||
// we should also accept a raw machine as actor logic here | ||
// or just make machine actor logic | ||
export type Spawner = <T extends ActorLogic<any, any> | string>( // TODO: read string from machine logic keys | ||
export type Spawner = <T extends AnyActorLogic | string>( // TODO: read string from machine logic keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: improve spawn
@Andarist @davidkpiano I don't know if you already know the following or not, I'm not assuming and I don't mean to offend, I'm just trying to help 😄 There are two ways that I know of of covariance and contravariance being properly expressed within typescript.
type Provider<out T> = () => T;
type Consumer<in T> = (x: T) => void;
type Mapper<in T, out U> = (x: T) => U;
type Processor<in out T> = (x: T) => T;
declare const Variance: unique symbol
interface Provider<T> {
[Variance]: {
_T: (_: never) => T
}
}
interface Consumer<T> {
[Variance]: {
_T: (_:T) => never
}
}
interface Mapper<T,U> {
[Variance]: {
_T: (_:T) => never
_U: (_: never) => U
}
}
interface Processor<T> {
[Variance]: {
_T: (_:T) => T
}
} The latter being especially useful to keep track of types generically without widening types or smashing into bivariance bugs in more complex cases of working with types. Like lifting, reversing, extracting, and deriving. Just to be clear, you're not constrained to functions, these concept extends to values just fine export const Variance = Symbol()
interface Left<E> {
readonly [Variance]: {
readonly _A: (_: never) => never;
readonly _E: (_: never) => E;
};
}
interface Right<A> {
readonly [Variance]: {
readonly _A: (_: never) => A;
readonly _E: (_: never) => never;
};
}
interface Both<E,A> {
readonly [Variance]: {
readonly _A: (_: never) => A;
readonly _E: (_: never) => E;
};
}
type Either<E,A> = Left<E> | Right<A>
type These<E,A> = Either<E,A> | Both<E,A> |
TS team considers announcement blog posts to be part of the documentation 🤷♂️ So this is "documented" in TS 4.7 blog post.
We already utilize a similar technique, for example here: xstate/packages/core/src/types.ts Line 122 in 4df5744
Although this one is more about helping the inference engine to recognize this as a potential inference source than about actually enforcing variance.
Don't be sorry! Every help is very much appreciated.
If this particular PR spurred this thought, then let me clarify what I'm doing in those tests. Type parameters have their own variance as long as they are actually used by structures. We rarely want to use runtime values as type containers (although at times that comes in handy and we might be using such tricks at some point. Those extensive~ tests were just added to ensure that the variance is correct. To some extent, we could just annotate type parameters with
|
As I said I'm not totally in the details, but I know there is some significant difference between // this:
interface Foo<A> {
_?: A
}
// and this:
interface Bar<A> {
_?: (_: never) => A
} The function signature notation is doing more than just signaling that it's covariant. I didn't mean to use properties as type containers, their shape is coincidental. I should have used only type lambas instead of indicating covariant/contravariant association. that was confusing of me. I've seen many phantom types around the repo. And I know you are well aware of functions changing how the engine determines type, as this quirk is utilized here: xstate/packages/core/src/types.ts Line 30 in 9911850
They can be both used together for greater good 😄 There is a way to avoid runtime representation using a symbol as the key for an optional field and not exporting it (basically expunging it from existence for everything besides types) I do think it's worth some exploration where this are applicable within xstate. A much more mature and comprehensive example than I am capable of producing can be found here: Theres example of extraction, unification (turning xstate is insanely smart, and I really think it straddles the edge of typescript in some cases. I don't think it should eschew more esoteric usage if users get more free stuff, especially if theres a compat concern |
I'm not aware of any difference. Not saying that it isn't there - TS sometimes acts differently based on details like this and the heuristics that it implements. So far though I didn't notice any drawbacks of the approach that we are using so I'm fine with sticking to it.
Well, that one is just copy-pasted from SO 🤣 but overall I'm pretty familiar with how TS works under the hood (learning more and more every day! I certainly don't have a perfect knowledge about those things).
I'm slightly worried about using symbols because they are nominal and staying away from them allows us to potentially avoid some problems with mismatched package versions etc.
Sure thing, I'm just not sure what does it solve for us today. If it solves some concrete problems, I would love to add tests showcasing those problems and using the alternative that would make them pass.
As I mentioned - I'm not strongly trying to avoid newer TS features. It's just that variance annotations aren't really strictly needed. You have to use those type parameters somewhere anyway because unused type parameters have unpredictable behaviors. And once type parameters are used they have their own variance already - annotations can't change them (well, they can make covariant/contravariant things invariant but that's not something that I'm looking for). In a way, those annotations should only be used as some kind of extra validation, like a lint rule. |
Thanks for the detailed responses @Andarist, I appreciate it greatly. I don't have too much free time, but if you can point me to some tests you'd like me to try and take a crack at, I'll be totally open to give it a go. |