Fix a few bugs related to custom events #256
Conversation
…ls and sent to the inspected source
|
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/statelyai/xstate-viz/2HPnRDiAuwNmdUQbSUgBrTxZa1YC |
| @@ -343,9 +343,13 @@ const newEventMachine = newEventModel.createMachine({ | |||
| on: { | |||
| '*': [ | |||
There was a problem hiding this comment.
It's weird that we accept any event here but you assert that the event is of type EVENT.PAYLOAD. In such a case, shouldn't this be changed to this?
| '*': [ | |
| 'EVENT.PAYLOAD': [ |
There was a problem hiding this comment.
However, by looking at the code - this wouldn't be exactly correct because other events should result in the invalid state. In such a case I would propose to do the whole try/catch thing in the if block within cond that would be responsible for that event to make this more clear. Something like:
if (e.type === 'EVENT.PAYLOAD') {
// do the try/catch thing
} else {
return false
}There was a problem hiding this comment.
Or you could do this:
on: {
'EVENT.PAYLOAD': [{ cond: () => { /* try/catch thing */ }, target: '.valid' }, '.invalid'],
'*': '.invalid'
}There was a problem hiding this comment.
This is definitely subject to refactoring but they're not changed in the scope of this PR 🤷♂️. Let's fix in a separate PR?
There was a problem hiding this comment.
This free flow of my mind (😅 ) has been prompted by the fact that this transition is defined for * events but it casts the received event to EVENT.PAYLOAD, seemingly not caring about other event types that can actually be received here.
This has been introduced in this PR and was not a problem before (because before the event type was not used here). So I think this should be refactored somehow as part of this PR
| event: { | ||
| ...e.event, | ||
| origin: e.event.origin || ctx.currentSessionId, | ||
| }, |
There was a problem hiding this comment.
I don't think this is necessarily the correct thing to do. In my mind sending events from the viz resembles this piece of code:
const service = interpret(machine).start()
onClick={() => service.send(createEvent())}And in this scenario, the origin wont be set: https://codesandbox.io/s/hopeful-elion-0l4vg?file=/src/index.js
So, from my perspective, in this basic and most common scenario this wont resemble the production usage and thus it might introduce confusing behavior.
I recognize how not setting origin in some other scenarios might be "incorrect" as well though and it's something I've been mentioning to David some time ago. We've concluded though that, for the most part, this should be a less common scenario and that we won't bother with it at the moment.
When this can be incorrect? When an event can only be received from another machine in the real application and when one uses the origin to do something (like responding to it). We can't currently determine possible sources though so we don't have a way to provide any hints about them (so they could be selected or something).
Keep in mind that sending events from the Viz to any non-root machine is often risky - in a sense that it might make the whole thing behave differently than in a production use case (unless your application literally is sending directly to such a machine, which is a valid use case). In a lot of scenarios, you could end up with some inconsistent state that way. One that would not be achievable by the real app.
There was a problem hiding this comment.
One of those scenarios in which not setting the origin is a breaking behavior is when we send a custom event to an external inspected source. How would we solve that without setting the origin to the current interpreted machine? Maybe conditionally doing it only if we're in the inspector mode?
There was a problem hiding this comment.
Could we go through this case on a call on Monday? I would love to get a little bit more context about this and this would allow me to jump into this quicker
There was a problem hiding this comment.
I would probably recommend dropping the toSCXMLEvent conversion from here:
xstate-viz/src/EventsPanel.tsx
Line 388 in 7cc57c8
and doing that here:
toSCXMLEvent(e.event, { origin: ctx.currentSessionId })
There was a problem hiding this comment.
Could we also add some basic tests for this thing?
It would also be nice if we could explain such changes, at least briefly. What has been previously assumed? What has happened? How it got fixed? And why the new approach is better? I had to figure this stuff out on my own when reviewing this - and having some kind of an explanation/reasoning behind the changes would help me a lot.
I'm on it. |
Fixes #250 #222 #223