-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Added the "eventType" in initialTransitionObject #5385
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
base: main
Are you sure you want to change the base?
Added the "eventType" in initialTransitionObject #5385
Conversation
|
hey @davidkpiano , if you have any comments over this you can ping me to update or we can proceed with the merge. |
// 🔧 Patch non-string eventType fields | ||
function fixEventTypes(obj: any) { | ||
if (Array.isArray(obj)) { | ||
obj.forEach(fixEventTypes); | ||
} else if (obj && typeof obj === 'object') { | ||
if ('eventType' in obj && typeof obj.eventType !== 'string') { | ||
obj.eventType = String(obj.eventType); | ||
} | ||
Object.values(obj).forEach(fixEventTypes); | ||
} | ||
} | ||
|
||
fixEventTypes(json); |
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 don't think this code should be here. If .eventType
is missing somewhere then please include it in the core of the library - not patch it up in a test file (and in a completely wrong way, from what I can tell).
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.
hey @Andarist , can you once go through all the changes I have made I think then you can get the reason of converting the nested eventType into string for test case so it passes the validate function even then you have some more better approach I would be happy to implement just drop it in thread. 😊
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.
In a way, this reverts part of #5380 . I'm not sure if that change was good or not but based on this PR alone I have no idea how to assess what you are after here. Please provide a full e2e test/description of what happens right now and how things go in your way.
Stringifying an arbitrary value and assigning it back to an object in this test code is definitely not a solution to any problem.
Perhaps you have a gripe with this thing here?
xstate/packages/core/src/StateNode.ts
Line 248 in cd9af19
eventType: null as any, |
But even if that's the case, I still don't fully understand your problem so it would be great if you could describe it in full detail. Note that initial transitions shouldn't have a required eventType
property at all because they can be taken in response to just any event accepted by a machine. There is no singular event type that could be assigned to this property
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 followed his instructions while solving the issue @Andarist
fixes: #5368
fixed the type error for the initialTransitionObject added the required "eventType" support.