Skip to content

[v5] Actions refactor#2484

Merged
davidkpiano merged 76 commits intonextfrom
v5/actions-refactor
Nov 12, 2021
Merged

[v5] Actions refactor#2484
davidkpiano merged 76 commits intonextfrom
v5/actions-refactor

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