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

Conversation

farskid
Copy link
Contributor

@farskid farskid commented Sep 8, 2021

Fixes #250 #222 #223

@changeset-bot
Copy link

changeset-bot bot commented Sep 8, 2021

⚠️ No Changeset found

Latest commit: 7cc57c8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Sep 8, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/statelyai/xstate-viz/2HPnRDiAuwNmdUQbSUgBrTxZa1YC
✅ Preview: https://xstate-viz-git-farskid-sta-578-send-button-is-0c3dfd-statelyai.vercel.app

@linear
Copy link

linear bot commented Sep 8, 2021

@farskid farskid marked this pull request as draft September 8, 2021 16:41
@farskid farskid self-assigned this Sep 8, 2021
@farskid farskid marked this pull request as ready for review September 8, 2021 19:10
@@ -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

Comment on lines +306 to +309
event: {
...e.event,
origin: e.event.origin || ctx.currentSessionId,
},
Copy link
Member

@Andarist Andarist Sep 9, 2021

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:

const scxmlEvent = toSCXMLEvent({

and doing that here:

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

Copy link
Member

@Andarist Andarist left a comment

Choose a reason for hiding this comment

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

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.

@farskid
Copy link
Contributor Author

farskid commented Sep 9, 2021

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.

@davidkpiano
Copy link
Member

@farskid @Andarist Besides tests, what are the remaining changes here?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Send" button is disabled when trying to send an event with custom data
3 participants