[xstate] Actor typing improvements

[davidkpiano]

Addresses #1534

[changeset-bot]

🦋 Changeset detected

Latest commit: 89f9c27

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

This PR includes changesets to release 5 packages

Name	Type
@xstate/react	Minor
xstate	Minor

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

[Andarist]

I've tried to apply this patch on the original issue and either I'm doing something wrong or it's not quite fixing the reported problem:
https://codesandbox.io/s/thirsty-goldberg-z471l?file=/src/App.tsx

[davidkpiano]

@Andarist Just made some massive improvements to the types, can you re-review?

[Andarist]

what is the desired interface for an actor and its subscribe method? Right now this extends Subscribable that required subscribe to implement both its overloads - it seems pretty limiting 🤔 could we change the Subscribable to be a union of its current overloads?

[davidkpiano]

It currently does need to implement both overloads, though. This is the same expected behavior as RxJS: https://github.com/ReactiveX/rxjs/blob/master/src/internal/Observable.ts#L71-L78 and the tc39 Observable spec:

interface Observable {

    // ...

    // Subscribes to the sequence with an observer
    subscribe(observer : Observer) : Subscription;

    // Subscribes to the sequence with callbacks
    subscribe(onNext : Function,
              onError? : Function,
              onComplete? : Function) : Subscription;

    // ...
}

[Andarist]

Right, it seems a little bit limiting for users to implement both. Take a look at this simple example:
https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgPICMDO0Bu0A8AKgHzIDeAUMsiBAB5gBcyAFDnADYCuEzhAlMgC8pHAHtgAEwDcVZNChiozFguZwQAT0Ejk4qbOoIxAWwAOHCJBU7REmRQC+FCqEixEKAMpcsCKMBmYMBiIORyXCCYvpj+wOgQLPzM+g7OruDQ8EjIPn4B6HDolkSklNTR+fGJYli40MwY2FB4UKXJuTFxQSEghsiVsQWJctS0DCrs3LzIAsJ2UgA0o-JQilAA-CoKSupatnr2y9RGphZWEFusB6lyHXlDgcGhsuluWZ7IAIIIYEoAShAYEQAKJ4cDyBgQECSTDkZBgTRmGaYMABEAAc2QjkWsxBJmAYEgkmEyA0mmIcnokBhcIecUKxQgoIJRIgkjKcmwMO24KYeL5N3srxcFBgkV+vTJCCQQR+fygLEQCuY8oBQPw5Nx5OIgjI6WMUTA0oVpPKJ25klUfOYZARSJmAHIAGKoVCO7F6lwnbHHC1dYYscb8ticZggLgmBJQIVSL0rZzpRCysBqxXKpT8IA

I would expect as a user this to work - XState itself will only ever call .subscribe using a particular signature (it doesn't matter which one it is right now). If I, as the actor author, want to support multiple signatures so it could be used by different consumers then this is fine - it's up to me, I can do that. However, right now it is required from me to support all of them which brings some boilerplate and ceremony that could be avoided.

I don't have a strong opinion about this - just something I've noticed while playing around with this branch.

[davidkpiano]

However, right now it is required from me to support all of them which brings some boilerplate and ceremony that could be avoided.

Can you show me some example code where that boilerplate is occurring? The main use-cases regarding this are .subscribe(...), as in consuming a subscribable object, which does not require anything of the developer.

Right now, the actors are assumed to come from invoke or spawn(), which will already do the necessary work for a compatible .subscribe(...). The use-case of creating an actor externally and using that within useActor(...) seem to be very edge-case.

We can make .subscribe() an optional property, though 🤔

[davidkpiano]

Also, we can make the internal toObserver(...) function a helper function:

const subscribe = (a, b, c) => {
  const observer = toObserver(a, b, c);

  // observer is an object with { next, error, complete }
}

[Andarist]

The boilerplate/ceremony part is showcased in the playground in my comment (TS playground).

It's not possible to create a very simple and lightweight actor that would match our expectations. The toObserver util is just what would make it slightly easier for developers but is a part of the ceremony I'm talking about. It wouldn't be sufficient though because one has to still type arguments but maybe something like this would be enough to make work:

const subscribe: Subscribable<T>["subscribe"] = (a, b, c) => {
  const observer = toObserver(a, b, c);

  // observer is an object with { next, error, complete }
}

It ain't super straightforward to always type this using the provided Subscribable - it depends on how do u write your code, for example, an object's property is not that easy (intuitive) to type using this.

But I get that usually people are not supposed to write actors by hand, although it would be nice if this would be supported out of the box.

We can make .subscribe() an optional property, though 🤔

It's not really about it being optional or not - I would like to actually provide it, just with simpler types.

---

This is not a blocking comment by any means - just something that I've observed while reviewing this. Food for thought and maybe not even smth that should be changed - I still think it was OK to raise this concern, after all, we want to primarily focus on actors and their interfaces, compatibility with different data types (like observables) should come second. I don't believe that consuming actors as observables is really that popular of a use case 🤔

[davidkpiano]

It's not really about it being optional or not - I would like to actually provide it, just with simpler types.

We can either provide it with simpler types and lose compatibility with RxJS and other libraries that implement the tc39 spec (proposal), or we can maintain full compatibility. Let's aim for the latter.

For the developer wishing to make a compatible actor, the "boilerplate" types should be invisible to them (not directly imported). At most, the only thing they would need to import is the Observer type:

import { Observer } from 'xstate';

const actor = {
    send(event: { type: 'FOO' }) {

    },
    subscribe(next: Observer<number> | ((val: number) => void)) {
      return { unsubscribe: () => {} }
    }
}

acceptActor(actor)

TS Playground

[Andarist]

I've retried this on the original case reported by the user (see) and everything looks great, great job 👍

Just dont forget to add changesets for this. Docs regarding stuff like ActorRefFrom would also be helpful.