mattpocock / xstate-catalogue

Professionally designed, interactive state machines
https://xstate-catalogue.com
MIT License
814 stars 63 forks source link

Events in sidebar not working for drag and drop, pagination and offline queue machines #15

Open bemayr opened 3 years ago

bemayr commented 3 years ago

While working in #13 I stumbled upon a problem with the current eventPayloads implementation.

The events in this Statechart are parametrized with the respective index that is either dragged (PICK_UP) or reaches an intersection (DRAG_REACHED_INTERSECTION). This works pretty well here:

## Test cases

### Swapping items 1 and 2

Pick up first item: <Event index={0}>PICK_UP</Event>.

Cross over second item: <Event index={1}>DRAG_REACHED_INTERSECTION</Event>.

Drop item: <Event>DROP</Event>.

Numbers one and two of the data list should have swapped places.

### Swapping items 1 and 3

Pick up first item: <Event index={0}>PICK_UP</Event>.

Cross over third item: <Event index={3}>DRAG_REACHED_INTERSECTION</Event>.

Drop item: <Event>DROP</Event>.

The first item should now be at the end.

but sadly does not work in the Sidebar. (because of the way events are distinct by their name only)

Result

The interactive documentation is one of the best features I have seen lately, maybe we can find a way with great UX to provide an event's payload, or David is already working/has already done this (😉). 🚀

Affected Machines


edits:

hmaurer commented 3 years ago

I think the current eventPayloads implementation does work with the sidebar? e.g. https://github.com/mattpocock/xstate-catalogue/blob/4e730ab1d5194224d22bfcef75844327650dd084/lib/machines/debounce.mdx exports eventPayloads, and it works with events in the sidebar. The drag-and-drop machine, however, does not export eventPayloads.

Also, trying to reproduce this issue I noticed that the issue you highlighted above occurs in the visualiser (when clicking on events), regardless of whether eventPayloads is exported or not!

mattpocock commented 3 years ago

@hmaurer Yeah this discussion should give us more info...

https://github.com/davidkpiano/xstate/discussions/2068

@bemayr There might be something more imaginative we can do here. A small form to submit a custom event payload?

<EventForm.Wrapper type="PICK_UP">
  <EventForm.NumberInput name="index" defaultValue={0} />
</EventForm.Wrapper>
bemayr commented 3 years ago

Yes exactly eventPayloads is already working. Matt implemented this after we discussed it in #6. I am now working on #13 where I add eventPayloads to all machines that need it and do not currently expose it. The problem with the drag-and-drop machine is, that event payload can not be statically defined, because inside the documentation different payload values are used (e.g. DRAG_REACHED_INTERSECTION with an index of 1 as well as 3 as can be seen above.

So we would need to display an event more than once in the sidebar, because it can be parametrized. It could look like DRAG_REACHED_INTERSECTION(1) and DRAG_REACHED_INTERSECTION(3) but I based on my experience this might confuse users, because it is still the same event, because matching occurs based on the name (if no guards are specified), but now it would be listed multiple times.

And regarding the visualizer, yes you are completely right. The visualizer knows nothing about eventPayloads, that is specific to xstate-catalogue only, the visualizer operates solely on the Statechart definition provided via inspect(...). You can provide custom payloads in the visualizer using the Events-tab: image

Although the Debounce Statechart is a special case here. See: https://github.com/mattpocock/xstate-catalogue/discussions/14

bemayr commented 3 years ago

@bemayr There might be something more imaginative we can do here. A small form to submit a custom event payload?

<EventForm.Wrapper type="PICK_UP">
  <EventForm.NumberInput name="index" defaultValue={0} />
</EventForm.Wrapper>

Oh yes of course, this is going in the right direction. I love the wording more imaginative! 😁 Let me be a little bit imaginative here (this was actually one of the problems/ideas I had in mind at my former company where my thesis topic originates):

If we get access to the event's type information we could render a form based on the parameters needed to create a sendable event. So for primitive types we could automatically render form input fields and everything more complex could fall back to a JSON Input as can be seen in David's inspector. Maybe this idea is over-engineering this problem for now, but in my opinion it could make an awesome tool for explorative testing of Statecharts that are a little bit more complex.

hmaurer commented 3 years ago

@bemayr that's a neat idea; leveraging types is always satisfying! Note however that neither the form nor the JSON input will work for functions (e.g. action in the debounce machine). I don't know if that's a concern or not.

A simpler but perhaps sufficient alternative would be to allow for multiple event payloads to be defined, each identified by a name (i.e. "variants" of events). Then when clicking on an event you could select the payload you want to send (with a dropdown).

hmaurer commented 3 years ago

This makes me wonder whether it's reasonable at all to have functions as part of event payloads. I am not too familiar with xstate "best practices"; is there a recommendation / requirement that events are serialisable? c.f. https://github.com/mattpocock/xstate-catalogue/discussions/14#discussioncomment-576257

bemayr commented 3 years ago

This makes me wonder whether it's reasonable at all to have functions as part of event payloads. I am not too familiar with xstate "best practices"; is there a recommendation / requirement that events are serialisable?

Came to the same conclusion and started https://github.com/mattpocock/xstate-catalogue/discussions/14 because of that.

mattpocock commented 3 years ago

@bemayr I think we should be more explicit with the event payloads. This auto-generation of event payloads from the schema is in the works - you can actually define your events using JSON Schema as of 4.17.0. I know David is thinking about this a lot for the visual builder - autogenerating events will probably be a first class citizen in the future.

https://github.com/davidkpiano/xstate/releases/tag/xstate%404.17.0

This project isn't the space for this sort of innovation, though - because it's a learning resource, we should be more transparent and less magical with how we declare event payloads.

@hmaurer As far as I know, there's no requirement for event payloads to be JSON-serialisable in the same way as, say, Redux recommends.

I do like the idea of variants of events - though this may end up being less useful than a small form where a user can define any payload of an event themselves.

bemayr commented 3 years ago

Ah yes, I completely get that - thanks for the detailed explanation! Then I totally ship the idea of the small form, because as you said in this project it does not have to be fully generic and more transparency yields a better learning effect!

And regarding the JSON serializability I am afraid to tell you that it is like in redux: https://github.com/mattpocock/xstate-catalogue/discussions/14#discussioncomment-576257 But I think we should definitely clarify why it is a "should" and not a "must", because as can be seen in your example, it works? 🤔

hmaurer commented 3 years ago

Regarding the idea of small forms and the mention of JSON Schema in xstate@4.17.0, I came across this project recently which might be of interest: https://github.com/rjsf-team/react-jsonschema-form

mattpocock commented 3 years ago

These are sweet ideas, and we can probably initiate a discussion about them on the discussions section. I've already got some nice designs fizzing around in my head.

What was the original scope of this issue? That the sidebar event payloads don't work? @bemayr

bemayr commented 3 years ago

What was the original scope of this issue? That the sidebar event payloads don't work? @bemayr

It originated out of #13 where I was unable to fix it for all the machines. So I fixed it for all possible ones with #16. The ones where it is still not working are:

Shall we open a new issue for those (to keep track) and close this one?


Looking forward to your design ideas!

mattpocock commented 3 years ago

Let's rename this one, so that we keep the context above.