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] Input #3815

Merged
merged 34 commits into from
Mar 17, 2023
Merged

[v5] Input #3815

merged 34 commits into from
Mar 17, 2023

Conversation

davidkpiano
Copy link
Member

@davidkpiano davidkpiano commented Feb 7, 2023

This PR adds input to the second argument of the interpret(...) function. This passes input data in the "xstate.init" event:

const greetMachine = createMachine({
  context: ({ input }) => ({
    greeting: `Hello ${input.name}!`
  })
  // ...
});

const actor = interpret(greetMachine, {
  // Pass input data to the machine
  input: { name: 'David' }
}).start();

Addresses STA-3438

@changeset-bot
Copy link

changeset-bot bot commented Feb 7, 2023

🦋 Changeset detected

Latest commit: 718da10

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

This PR includes changesets to release 1 package
Name Type
xstate Major

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 7, 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 718da10:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration

@ghost
Copy link

ghost commented Feb 7, 2023

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

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

packages/core/src/types.ts Outdated Show resolved Hide resolved

const Test = () => {
const [state] = useMachine(testMachine, {
input: { test: true }
Copy link
Member

Choose a reason for hiding this comment

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

we should add the same test to all other integration packages

Copy link
Member Author

Choose a reason for hiding this comment

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

@with-heart
Copy link
Contributor

Are you thinking of adding a way to get/set the type of input for a machine?

@Andarist
Copy link
Member

Andarist commented Mar 2, 2023

As in with something like withInput? I think that's not necessary. A machine is like a class and input is information specific to its instance.

@with-heart
Copy link
Contributor

More in the sense of making it part of a machine's public api so you can expect a certain type of input data that's consumed

@Andarist
Copy link
Member

Andarist commented Mar 3, 2023

Ah, ye - the types for this will come in a followup PR

Base automatically changed from v5/recursive-persistence to next March 14, 2023 12:04
Comment on lines 101 to 103
// TODO: this getter should be removed
public get context(): TContext {
return this.getContextAndActions()[0];
public getContext(input?: any): TContext {
return this.getContextAndActions(input)[0];
Copy link
Member

Choose a reason for hiding this comment

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

thoughts about removing this getter altogether?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm okay with it as long as we can make all the tests pass still 😅

Copy link
Member

Choose a reason for hiding this comment

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

let's do this in a follow up work as this is just a symptom of a bigger story

string,
ActorBehaviorCreator<TContext, TEvent> | AnyActorBehavior
>;
actors: Record<string, AnyActorBehavior>;
Copy link
Member

Choose a reason for hiding this comment

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

As we might see in the diff here - now actors won't have access to TContext and TEvent. So it's not possible to "configure" their dynamic part (input) in the implementations object. That's only possible within the config itself, through the invoke.input property. However, at the moment, we are not able to provide typegen-based information there so this might get annoying to people.

WDYT about allowing this to be:

// it's not a complete final type, just an illustration for this accepting `{ src, input }` 
actors: Record<string, AnyActorBehavior | { src: AnyActorBehavior, input: InputCreator<TContext, TEvent> }>; 

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that input might be different depending on where you come from:

state1: {
  invoke: {
    src: 'something',
    input: (ctx) => /* get from context */
  }
}

state2: {
  invoke: {
    src: 'something',
    input: (_ctx, ev) => /* get from event */
  }
}

I don't understand the typegen information; invoke.input is only relevant to the invoked actor, not to the machine (which is the scope of typegen); can you clarify?

Copy link
Member

Choose a reason for hiding this comment

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

The conceptual TInput always comes from a given actor type and is the same regardless of where and how it is instantiated. What might be different between instantiation sites, are the "arguments" used to resolve that same type. But that's exactly the same as with any other implementation type:

state1: {
  entry: 'something'
}

state2: {
  entry: 'something'
}

The final implementation of something just has to deal with all possible argument types. It's free to ignore context completely, and it's free to ignore event - but when it starts using the event, it just has to assume that it's a union of all possible event types that can lead to triggering this specific action.

Copy link
Member Author

Choose a reason for hiding this comment

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

What should be the resolution for this?

Copy link
Member

Choose a reason for hiding this comment

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

I propose this: #3897

davidkpiano and others added 2 commits March 17, 2023 06:42
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
* Allow input to be referenced in the implementations

* Allow dynamic parametrized inputs to be used together with spawn

* make tests less dirty

Co-authored-by: David Khourshid <davidkpiano@gmail.com>

* Update packages/core/src/StateMachine.ts

---------

Co-authored-by: David Khourshid <davidkpiano@gmail.com>
@Andarist Andarist merged commit 66bc88a into next Mar 17, 2023
@Andarist Andarist deleted the v5/input-3 branch March 17, 2023 12:23
@Andarist Andarist mentioned this pull request Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants