-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
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 |
@@ -365,7 +354,7 @@ export function formatTransition< | |||
} | |||
const transition = { | |||
...transitionConfig, | |||
actions: toActionObjects(toArray(transitionConfig.actions)), |
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.
Why is toActionObjects
no longer called? A string "action"
should be converted to { type: 'action' }
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 is also what is causing the test failures, I believe
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the time being I fixed the JSON serialization - changes around this can be moved to other discussions/PRs.
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.
Some nits, otherwise LGTM
No description provided.