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

Required input when defined inside types #5055

Merged

Conversation

SandroMaglione
Copy link
Contributor

@SandroMaglione SandroMaglione commented Aug 25, 2024

This PR updates the types to require input when defined inside types/input when using useActor, useMachine, and useActorRef.

Rationale

With the current types even if types/input is defined, when using hooks like useActor the input 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 provide input 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, but useActorRef has a third parameter observerOrListener, which does not allow to spread options to make them optional:

...[options]: ConditionalRequired<
[
options?: ActorOptions<TLogic> & {
[K in RequiredOptions<TLogic>]: unknown;
}
],
IsNotNever<RequiredOptions<TLogic>>
>

export function useActorRef<TLogic extends AnyActorLogic>(
machine: TLogic,
options: ActorOptions<TLogic> = {},
observerOrListener?:
| Observer<SnapshotFrom<TLogic>>
| ((value: SnapshotFrom<TLogic>) => void)
): Actor<TLogic> {

I opted instead to make input required while options is still optional. This makes input required when passing options, but if options is not passed the type will work even if types/input is defined:

export function useActorRef<TLogic extends AnyActorLogic>(
machine: TLogic,
options?: ConditionalRequired<
[
options?: ActorOptions<TLogic> & {
[K in RequiredOptions<TLogic>]: unknown;
}
],
IsNotNever<RequiredOptions<TLogic>>
>['0'],
observerOrListener?:
| Observer<SnapshotFrom<TLogic>>
| ((value: SnapshotFrom<TLogic>) => void)
): Actor<TLogic> {

A solution may be to merge observerOrListener inside options. This may be a breaking change.

Copy link

changeset-bot bot commented Aug 25, 2024

🦋 Changeset detected

Latest commit: 08932cd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
xstate Patch
@xstate/svelte Patch
@xstate/react Patch
@xstate/solid Patch
@xstate/vue Patch

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

export * from './actions.ts';
export * from './actors/index.ts';
export { assertEvent } from './assert.ts';
export {
Actor,
createActor,
interpret,
type Interpreter
type Interpreter,
type RequiredOptions
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opted for RequiredActorInstanceOptions

@Andarist
Copy link
Member

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, but useActorRef has a third parameter observerOrListener, which does not allow to spread options to make them optional:

You can create [options?, observerOrListener?], make the first element required conditionally and spread the result. This way you will make the second argument conditionally required while keeping the third one optional

@SandroMaglione
Copy link
Contributor Author

You can create [options?, observerOrListener?], make the first element required conditionally and spread the result. This way you will make the second argument conditionally required while keeping the third one optional

@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 A rest element must be last in a destructuring pattern.ts(2462):

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?

@Andarist
Copy link
Member

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.

Copy link
Member

@Andarist Andarist left a 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)

@SandroMaglione SandroMaglione force-pushed the sandro/required-input-when-defined branch from 5fb1591 to 1ea6745 Compare August 26, 2024 17:31
@SandroMaglione
Copy link
Contributor Author

@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.

@Andarist
Copy link
Member

Andarist commented Sep 5, 2024

LGTM - I just left one comment to address

@Andarist Andarist merged commit ad38c35 into statelyai:main Sep 6, 2024
1 check passed
@github-actions github-actions bot mentioned this pull request Sep 6, 2024
@pckilgore
Copy link

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?

@Andarist
Copy link
Member

@pckilgore please provide a repro case so we can take a look at your case

@SandroMaglione
Copy link
Contributor Author

@pckilgore if your input is in the form { input: { value?: string; }} it will result required since only value is optional, whereas input is required to be defined

@pckilgore
Copy link

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.

@pckilgore
Copy link

@pckilgore if your input is in the form { input: { value?: string; }} it will result required since only value is optional, whereas input is required to be defined

That would have still been odd, because I would have expected typescript to accept { input: {} } in that circumstance, not just {} as the second parameter. Appreciate the suggestion!

@SandroMaglione
Copy link
Contributor Author

Yes, I would expect { input: {} } as well. Was it working with just {}? If so I wouldn’t be sure, definitely a repro is needed

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

Successfully merging this pull request may close these issues.

3 participants