-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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] implement action groups #3997
Conversation
🦋 Changeset detectedLatest commit: 8a38c23 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
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 8a38c23:
|
b2b4224
to
96307d0
Compare
96307d0
to
aa6206f
Compare
.changeset/wild-roses-sort.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was worried this might be too long but after thinking about it, I think this is probably the minimum explanation I can come up with to really get to the heart of what's cool about this feature
export type ActionGroup< | ||
TContext extends MachineContext, | ||
TExpressionEvent extends EventObject, | ||
TEvent extends EventObject = TExpressionEvent, | ||
TActionTypes = string | ||
> = Array< | ||
| TActionTypes | ||
| ParameterizedObject | ||
| BaseDynamicActionObject< | ||
TContext, | ||
TExpressionEvent, | ||
TEvent, | ||
any, // TODO: this should receive something like `Cast<Prop<TIndexedActions, K>, ParameterizedObject>`, but at the moment builtin actions expect Resolved*Action here and this should be simplified somehow | ||
any | ||
> | ||
| ActionFunction< | ||
TContext, | ||
TExpressionEvent, | ||
ParameterizedObject, // TODO: when bringing back parametrized actions this should accept something like `Cast<Prop<TIndexedActions, K>, ParameterizedObject>`. At the moment we need to keep this type argument consistent with what is provided to the fake callable signature within `BaseDynamicActionObject` | ||
TEvent | ||
> | ||
>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding action groups to MachineImplementationsActions
was getting pretty messy so I pulled this out as a separate type.
params: { | ||
actions: toActionObjects(dereferencedAction).map((object) => ({ | ||
...object, | ||
params: { | ||
...object.params, | ||
...params | ||
} | ||
})) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since each action needs to receive params
, I just resolved the action objects for each here and then mapped them to include the params.
Is that fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me 👍
entry: { | ||
type: 'messageHandlers', | ||
params: { | ||
message: 'Hello world!' | ||
} | ||
} | ||
}, | ||
{ | ||
actions: { | ||
messageHandlers: [ | ||
'handler1', | ||
{ type: 'otherHandlers', params: { level: 'info' } } | ||
], | ||
otherHandlers: ['handler2'], | ||
handler1: ({ action }) => handler1(action.params), | ||
handler2: ({ action }) => handler2(action.params) | ||
} | ||
} | ||
); | ||
|
||
interpret(machine).start(); | ||
|
||
expect(handler1).toHaveBeenCalledWith({ message: 'Hello world!' }); | ||
expect(handler2).toHaveBeenCalledWith({ | ||
message: 'Hello world!', | ||
level: 'info' | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has an implicit test for params
overriding. Should we be more explicit with a separate test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might not be a need because if the behavior changes, this test will fail.
cc. @Andarist for his thoughts
I didn't take a thorough look through the implementation but here are my thoughts:
|
That's fine with me!
That makes sense to me. I'll work on getting the typegen PR updated so it can land before this. |
🧹 I think that with // TENTATIVE API
const machine = setup({
actions: {
actionGroup: (_, x) => {
x.action({ type: 'action1' });
x.action({ type: 'action2' });
},
action1: () => { ... },
action2: () => { ... },
}
}).createMachine({
entry: 'someGroup'
}); |
@davidkpiano that sounds lovely! Do you have v6 ideas documented anywhere publicly? I'd love to see what you're thinking |
Not yet, but once I set up a branch/draft PR for it I'll let you know. |
Thank you! ❤️ |
This PR implements #3996, enabling actions to be implemented as an action group. (replaces #3561)
An action group is a named array of actions. When the action group is executed, each action in the group is executed in order:
Action groups allow us to avoid error-prone repetition of actions, instead defining the group once and reusing it anywhere—like a single source of truth for the algorithm the group represents:
Action groups can reference other action groups by name. The referenced group's actions will be executed in order from the point of reference—like spreading the referenced group's actions in the group:
With a mix of actions, action groups, and action group references, we can compose our algorithms in flexible and reusable ways:
I feel like there's something powerful to thinking about action groups as composable algorithms, though I'm not entirely sure what that is yet, so I'm excited for this to land so more people can play with it!