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] Actions refactor #2484

Merged
merged 76 commits into from
Nov 12, 2021
Merged

[v5] Actions refactor #2484

merged 76 commits into from
Nov 12, 2021

Conversation

davidkpiano
Copy link
Member

@davidkpiano davidkpiano commented Jul 31, 2021

This PR reworks actions so that there are "dynamic actions" and base action objects. This improves type inference for built-in actions, but also requires that parameterized actions have a params property:

{
  type: 'someType',
  params: {
    one: { ... },
    two: { ... }
  }
}

@devanshj
Copy link
Contributor

devanshj commented Aug 5, 2021

Typing action and especially action meta was a problem in txstate, haven't checked but I think this is going to make it much much much easier, so super cool change :P

@devanshj
Copy link
Contributor

devanshj commented Aug 5, 2021

Also I assume guards will also have params?

@davidkpiano
Copy link
Member Author

Typing action and especially action meta was a problem in txstate, haven't checked but I think this is going to make it much much much easier, so super cool change :P

Yes, action objects were previously a mess; this is going to make it much better.

Also I assume guards will also have params?

Yes, guards have params 👍

@devanshj
Copy link
Contributor

devanshj commented Aug 5, 2021

That's great!

@devanshj
Copy link
Contributor

devanshj commented Aug 5, 2021

Wait a min looked at the code, it's not exactly what I expected xD

I was expecting ActionObject to not allow arbitrary keys on it, only on params. So I was expecting a diff like this...

 type ActionObject = {
   type: string,
   exec?: ...,
-  [k: string]: unknown
+  params: Record<string, unknown>
 }

The fact that out of all keys only type and exec has a constraint is a major pain point, it seems the current change wouldn't help in typing much unfortunately haha.

Can we have my change? I know this would be breaking but I guess yours too is somewhat breaking? So if we're breaking things let's get them right.

And the index signature on action object is problem not just for txstate but for xstate types too

@davidkpiano
Copy link
Member Author

Can we have my change? I know this would be breaking but I guess yours too is somewhat breaking? So if we're breaking things let's get them right.

Which change is this?

Should we only allow inline action objects like this?

{
  type: string;
  params: { ... };
  // no extra props
}

@davidkpiano davidkpiano mentioned this pull request Nov 8, 2021
@davidkpiano davidkpiano merged commit 835b996 into next Nov 12, 2021
@davidkpiano davidkpiano deleted the v5/actions-refactor branch November 12, 2021 13:26
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.

3 participants