Refactor how builtin actions are structured and how all actions are stored and resolved#4127
Refactor how builtin actions are structured and how all actions are stored and resolved#4127
Conversation
🦋 Changeset detectedLatest commit: bfa0e30 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 bfa0e30:
|
|
Looked mainly at the tests; LGTM but CI needs to pass |
| } | ||
| const transition = { | ||
| ...transitionConfig, | ||
| actions: toActionObjects(toArray(transitionConfig.actions)), |
There was a problem hiding this comment.
Why is toActionObjects no longer called? A string "action" should be converted to { type: 'action' }
There was a problem hiding this comment.
This is also what is causing the test failures, I believe
There was a problem hiding this comment.
Why is toActionObjects no longer called? A string "action" should be converted to { type: 'action' }
Why? :p I mean, this isn't really strictly needed for quite anything (as showcased by this PR). I could add back this particular conversion but I wonder if it's worth it. From the types PoV, this makes the publicly accepted action different from the one that we operate on internally and right now both can just use Action and it's pretty easy to work with.
This is also what is causing the test failures, I believe
One test was related to the lack of .type on builtin actions - just fixed it. There wasn't any test that would test that actions are called with an object form of the action even if it was configured as string though - just added one and pushed out a fix for it.
The second one is related to JSON schema serialization. I didn't exactly dig into what has changed and what it would take to fix it right now. I think it's worth considering just removing this test for now (and perhaps the JSON schema as well). We can always bring this back later but right now this relies on the fact that actions are always objects but with this PR they are not. I think it would be better to remove toJSON methods and implement converting logic in something akin to machineToJson. It would also be good to specify what's the goal of this JSON schema and what one could actually do with it today.
There was a problem hiding this comment.
For the time being I fixed the JSON serialization - changes around this can be moved to other discussions/PRs.
davidkpiano
left a comment
There was a problem hiding this comment.
Some nits, otherwise LGTM
No description provided.