-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Required input
when defined inside types
#5055
Required input
when defined inside types
#5055
Conversation
🦋 Changeset detectedLatest commit: 08932cd The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
packages/core/src/index.ts
Outdated
export * from './actions.ts'; | ||
export * from './actors/index.ts'; | ||
export { assertEvent } from './assert.ts'; | ||
export { | ||
Actor, | ||
createActor, | ||
interpret, | ||
type Interpreter | ||
type Interpreter, | ||
type RequiredOptions |
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 type should get renamed to be more specific if we want to export it
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.
export type RequiredOptions<TLogic extends AnyActorLogic> =
undefined extends InputFrom<TLogic> ? never : 'input';
Something like ActorHasRequiredInputOption
?
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 returns a union of required options - not a boolean. So I'd go with RequiredActorInstantiationOptions
or smth like this.
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.
Opted for RequiredActorInstanceOptions
You can create |
@Andarist not sure this works. I tried various implementations, but the issue is that making a parameter conditionally required requires to spread it, but spread is allowed only for the last parameter. This below doesn’t work export function useActorRef<TLogic extends AnyActorLogic>(
machine: TLogic,
...[...[options], observerOrListener]: [
options: ConditionalRequired<
[
options?: ActorOptions<TLogic> & {
[K in RequiredOptions<TLogic>]: unknown;
}
],
IsNotNever<RequiredOptions<TLogic>>
>,
observerOrListener?:
| Observer<SnapshotFrom<TLogic>>
| ((value: SnapshotFrom<TLogic>) => void)
]
): Actor<TLogic> These two solutions below make the parameter always required: export function useActorRef<TLogic extends AnyActorLogic>(
machine: TLogic,
...[options, observerOrListener]: [
options: ConditionalRequired<
[
options?: ActorOptions<TLogic> & {
[K in RequiredOptions<TLogic>]: unknown;
}
],
IsNotNever<RequiredOptions<TLogic>>
>,
observerOrListener?:
| Observer<SnapshotFrom<TLogic>>
| ((value: SnapshotFrom<TLogic>) => void)
]
): Actor<TLogic> export function useActorRef<TLogic extends AnyActorLogic>(
machine: TLogic,
...[options, observerOrListener]: [
ConditionalRequired<
[
options?: ActorOptions<TLogic> & {
[K in RequiredOptions<TLogic>]: unknown;
}
],
IsNotNever<RequiredOptions<TLogic>>
>['0'],
observerOrListener?:
| Observer<SnapshotFrom<TLogic>>
| ((value: SnapshotFrom<TLogic>) => void)
]
): Actor<TLogic> I also tried other options without much success. What do you think? |
Those type helpers are not too readable anyway today so I wouldn't try to reuse them at all costs. At the very least something along those lines should work: export function useActorRef<TLogic extends AnyActorLogic>(
machine: TLogic,
...[options, observerOrListener]: IsNotNever<
RequiredOptions<TLogic>
> extends true
? [
options: ActorOptions<TLogic> & {
[K in RequiredOptions<TLogic>]: unknown;
},
observerOrListener?:
| Observer<SnapshotFrom<TLogic>>
| ((value: SnapshotFrom<TLogic>) => void),
]
: [
options?: ActorOptions<TLogic>,
observerOrListener?:
| Observer<SnapshotFrom<TLogic>>
| ((value: SnapshotFrom<TLogic>) => void),
]
): Actor<TLogic>; I don't mind some mild repetition. |
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.
LGTM but before landing this, type-level tests should be added. (and it requires proper changesets)
5fb1591
to
1ea6745
Compare
@Andarist added type tests for required input and changeset. I marked this as a patch release, since I would consider this a bug fix. Let me know if this works. |
LGTM - I just left one comment to address |
My useMachine calls requires an empty object as a second parameter after this PR, even if there is no input required in any actors. Is that just the price I pay here? |
@pckilgore please provide a repro case so we can take a look at your case |
@pckilgore if your input is in the form |
I was able to fix by updating xstate itself to 5.19 from 5.18. Yarn didn't give me any peer dependency warnings about the mismatch, which isn't what I would have expected? Sorry to bother. |
That would have still been odd, because I would have expected typescript to accept |
Yes, I would expect |
This PR updates the types to require
input
when defined insidetypes/input
when usinguseActor
,useMachine
, anduseActorRef
.Rationale
With the current types even if
types/input
is defined, when using hooks likeuseActor
theinput
option remains optional.In my opinion this is an undesired behaviour, since it should be safe to assume that if the user defines an input it's expected to be provided when using the actor.
More than once I encountered runtime issues caused by forgetting to pass the input.
Solution
This PR copies the implementation of
createActor
that requires to provideinput
when defined.Issue with
useActorRef
It's probably not possible to make the same update to
useActorRef
for react and vue since the type is made optional by spreading options as the last function argument, butuseActorRef
has a third parameterobserverOrListener
, which does not allow to spreadoptions
to make them optional:xstate/packages/core/src/createActor.ts
Lines 792 to 799 in 1ab9745
xstate/packages/xstate-react/src/useActorRef.ts
Lines 45 to 51 in 1ab9745
I opted instead to make
input
required whileoptions
is still optional. This makesinput
required when passingoptions
, but ifoptions
is not passed the type will work even iftypes/input
is defined:xstate/packages/xstate-react/src/useActorRef.ts
Lines 55 to 68 in 10ed048
A solution may be to merge
observerOrListener
insideoptions
. This may be a breaking change.