Skip to content
This repository has been archived by the owner on Feb 25, 2024. It is now read-only.

Fix a few bugs related to custom events #256

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Fix a few bugs related to custom events #256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
450b577
set custom event origin to currentSessionId to be picked up by devtoo…
farskid Sep 8, 2021
7cc57c8
Fix bug with send button being disabled on valid JSON
farskid Sep 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/EventsPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,9 +343,13 @@ const newEventMachine = newEventModel.createMachine({
on: {
'*': [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Suggested change
'*': [
'EVENT.PAYLOAD': [

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you could do this:

on: {
  'EVENT.PAYLOAD': [{ cond: () => { /* try/catch thing */ }, target: '.valid' }, '.invalid'],
  '*': '.invalid'
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely subject to refactoring but they're not changed in the scope of this PR 🤷‍♂️. Let's fix in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
cond: (ctx) => {
cond: (_, e) => {
try {
const eventObject = JSON.parse(ctx.eventString);
const eventObject = JSON.parse(
(e as ReturnType<
typeof newEventModel.events['EVENT.PAYLOAD']
>).value,
);
return typeof eventObject.type === 'string';
} catch (e) {
return false;
Expand Down
5 changes: 4 additions & 1 deletion src/simulationMachine.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,10 @@ export const simulationMachine = simModel.createMachine(
(ctx, e) => {
return {
type: 'xstate.event',
event: e.event,
event: {
...e.event,
origin: e.event.origin || ctx.currentSessionId,
},
Comment on lines +306 to +309
Copy link
Member

@Andarist Andarist Sep 9, 2021 •
edited
Loading

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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably recommend dropping the toSCXMLEvent conversion from here:

xstate-viz/src/EventsPanel.tsx

Line 388 in 7cc57c8

const scxmlEvent = toSCXMLEvent({

and doing that here:

toSCXMLEvent(e.event, { origin: ctx.currentSessionId })

sessionId: ctx.currentSessionId,
};
},
Expand Down