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] Awaitable actors #3977

Closed
wants to merge 4 commits into from
Closed

[v5] Awaitable actors #3977

wants to merge 4 commits into from

Conversation

davidkpiano
Copy link
Member

This PR makes actors thenable; awaiting an actor will wait for output.

  const machine = createMachine({
    initial: 'a',
    states: {
      a: {
        after: {
          10: 'b'
        }
      },
      b: {
        type: 'final',
        output: { foo: 'bar' }
      }
    }
  });

  const output = await interpret(machine).start();

  expect(output).toEqual({ foo: 'bar' });

@davidkpiano davidkpiano requested a review from Andarist April 23, 2023 13:38
@changeset-bot
Copy link

changeset-bot bot commented Apr 23, 2023

⚠️ No Changeset found

Latest commit: b71269e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@davidkpiano davidkpiano changed the base branch from main to next April 23, 2023 13:38
@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 23, 2023

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 b71269e:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration

@ghost
Copy link

ghost commented Apr 23, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

}

// thenable
public then(
Copy link
Member

Choose a reason for hiding this comment

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

I don't "feel" this feature. If anything, I'd probably prefer smth like toPromise over this. Sure, we can make things "thenable" - should we though? The actor is not a promise and despite the fact that JS allows us to do this, it feels rather weird.

Since we already have waitFor I also don't necessarily feel the need for toPromise either. It's already simple to do this. Yet a different option would be to just have actor.waitFor.

Our ActorLogic interface starts to be fatter than a turkey on Thanksgiving 😬

Copy link
Member Author

@davidkpiano davidkpiano Jun 17, 2023

Choose a reason for hiding this comment

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

The current problem is there's no easy way to have "wait for actor to complete" for any actor logic, unless you're okay with an optional selector here:

// single argument - wait for actor to be done
const output = await waitFor(someActor);

// current behavior
// PROBLEM: not sure what should be returned
const output = await waitFor(someActor, state => /* ??? */);

What do you think would be a good workaround with waitFor?

Copy link
Member Author

@davidkpiano davidkpiano Aug 12, 2023

Choose a reason for hiding this comment

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

Ping @Andarist - need a good solution to this use-case:

// get actor output when the actor is completed
const output = await toPromise(actor);

Can't do this with waitFor().

@Andarist Andarist mentioned this pull request Aug 20, 2023
@davidkpiano
Copy link
Member Author

Superseded by #4198

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.

2 participants