statelyai / xstate

Actor-based state management & orchestration for complex app logic.
https://stately.ai/docs
MIT License
27.21k stars 1.26k forks source link

Require input to create `StateMachine` (`withInput`) #5049

Closed SandroMaglione closed 2 months ago

SandroMaglione commented 3 months ago

This PR adds a new withInput api to StateMachine. withInput allows to provide input to a state machine before creating an actor (i.e. outside of createActor):

createActor(machine, { input: { startCount: 42 } }).start();
createActor(machine.withInput({ startCount: 42 })).start(); // 👈 New API

[!NOTE] This change was proposed and discussed in Discord

changeset-bot[bot] commented 3 months ago

⚠️ No Changeset found

Latest commit: d86a3df4e7fd5f436684a863b182b73d28074d0a

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

Andarist commented 3 months ago

This is a breaking change. All machines that define types/input will not work anymore, since withInput becomes required.

As such we can't merge this into v5

davidkpiano commented 3 months ago

@SandroMaglione I believe that there is a way to make this not a breaking change 🤔

SandroMaglione commented 3 months ago

The current approach requires using withInput. In order for this to be no-breaking we would need to still return a machine, and add a withInput method to the machine itself

Not sure if that would help much, since in that case the current input api works more or less the same

I can take a look at how that would look like

Did you have other ideas?

davidkpiano commented 3 months ago

The current approach requires using withInput. In order for this to be no-breaking we would need to still return a machine, and add a withInput method to the machine itself

Let's try this approach

SandroMaglione commented 3 months ago

@davidkpiano updated this adding withInput to StateMachine. This allows to provide an input outside of createActor.

For now I added one new test for this:

https://github.com/statelyai/xstate/blob/a47d9e6de720d70e5289613ba12228aee84179f1/packages/core/test/input.test.ts#L33-L53

All other tests are working. This new implementation is no more a breaking change.

Let me know if this looks better, if so I will go ahead and add more tests to verify the new api works in other situations.

davidkpiano commented 3 months ago

@davidkpiano updated this adding withInput to StateMachine. This allows to provide an input outside of createActor.

For now I added one new test for this:

https://github.com/statelyai/xstate/blob/a47d9e6de720d70e5289613ba12228aee84179f1/packages/core/test/input.test.ts#L33-L53

All other tests are working. This new implementation is no more a breaking change.

Let me know if this looks better, if so I will go ahead and add more tests to verify the new api works in other situations.

This looks good to me! Just a little curious about the use of an extra class; if it's unavoidable, it's fine with me.

cc. @Andarist for your thoughts

SandroMaglione commented 3 months ago

This looks good to me! Just a little curious about the use of an extra class; if it's unavoidable, it's fine with me.

cc. @Andarist for your thoughts

The rationale for a new class is to type createActor. Adding input inside the original StateMachine makes the IsNotNever type check to always pass:

https://github.com/statelyai/xstate/blob/54c9d9e6a49ab8af8b58d700ed967536f9c06fb4/packages/core/src/createActor.ts#L790-L800

As such, createActor always required the input option. With the new class it's possible to type createActor with no options:

https://github.com/statelyai/xstate/blob/a47d9e6de720d70e5289613ba12228aee84179f1/packages/core/src/createActor.ts#L799-L818

If this type can be made not-required even with input inside StateMachine then the extra class would not be needed

Andarist commented 2 months ago

It would be great if the rationale for this new API would be included in the PR's description.

SandroMaglione commented 2 months ago

It would be great if the rationale for this new API would be included in the PR's description.

@Andarist @davidkpiano indeed I am not sure anymore that this change provides much value. The initial intention was to prevent creating a machine all together if the input was missing. But since that was a breaking change, it's not possible to release.

Therefore, I would opt to instead make the input option required when using hooks like useActor. I opened a PR for this #5055. I am going to close this. Feel free to reopen if you think this may be a desired change.

Andarist commented 2 months ago

I opened a PR for this https://github.com/statelyai/xstate/pull/5055.

I feel like the new one goes into the right direction 👍