[v5] Ensure that null events are not recorded

[davidkpiano]

This PR ensures that when taking eventless transitions, the resulting event that is read by subsequent guards and actions is the event that triggered the initial transition; not "null events" ({ type: '' })", which should not be recorded anyway.

Addresses https://github.com/statelyai/xstate/projects/1#card-34972159

[changeset-bot]

🦋 Changeset detected

Latest commit: 79b09a4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages

Name	Type
xstate	Major
@xstate/graph	Major
@xstate/immer	Major
@xstate/inspect	Major
@xstate/react	Major
@xstate/svelte	Major
@xstate/test	Major
@xstate/vue	Major

Not sure what this means? Click here to learn what changesets are.

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

[ghost]

CodeSee Review Map:

Review in an interactive map

View more CodeSee Maps

Legend

[Andarist]

sideways note -> what do you think about dropping support for TransitionsConfigArray entirely? This has been mainly introduced to support compatibility with SCXML better but it imho doesn't bring a lot of value and nobody is using this publicly. This isn't even documented.

[davidkpiano]

I'm wary to do this because an array offers interesting (and sometimes useful) patterns for resolving wildcard events.

[Andarist]

The wildcard case was partially a reason why i've added array support in the first place - after all that time I never really wanted to actually use this though, nor I've seen anyone using this. We are currently deprioritizing * in the object syntax (it's always matched after other transitions with exact descriptors) but the question is - should the same happen to other inexact descriptors, like foo.*? I'm on the fence here - if we shuffle priority orders on our own it seems a little bit like adding magic rules. If instead we would just trust the document order (for all descriptors, * included) then our implementation would get simpler but also the public behavior would be more predictable (we'd only have one rule -> document order).

While having an array support in place we can do more than with an object - question is: is this even a good thing in the first place? With time I'm getting more and more into KISS, as in - don't try to be interesting, let the code be dull and boring. Any clever solution can be refactored to a more dull one and the dull one will probably be easier to grasp over time. So I'm wondering if removing support for this wouldn't be actually a feature, forcing people not to use clever solutions.

It also seems that full array support is just harder to visualize - although document-order solution comes with similar problems, but at least the same descriptor cant be interleaved with other descriptors if we go with object-only syntax.

cc @johnyanarella @ChrisShank @ojkelly what are your thoughts on array support? how descriptor matching is working in StateML so far?

[davidkpiano]

Another use-case for a transition array: the planned feature of letting multiple events be handled by the same transition.

on: [
  { event: ['A_EVENT', 'B_EVENT', target: 'goHere' }
]

[ChrisShank]

@Andarist in John’s original DSL multiple events for a single transition was supported since it was largely compliant with SCXML. We have since removed this ability due to its apparently little to no usage. There are also potentially confusing semantics for a user that stems from whether to interpret the array of events as or or and. In other parts of the DSL an array means and (e.g. multiple targets) while this array means or. Additionally, there are also concerns of how to represent this feature in the visualization that implies the correct semantics.

By removing this feature we don't loss expressiveness since there is a simple rewrite that can represents the same model.

If there are legit use cases in the future we would reconsider adding this to the language.

[Andarist]

Another use-case for a transition array: the planned feature of letting multiple events be handled by the same transition.

IMHO, this sounds neat at the surface and I've also wanted to introduce this in the past. But now I don't think that introducing a syntax that makes you refactor the whole chunk of code is good. A good syntax should be "appendable" and focused, should allow local modifications. If we'd process event keys we could allow stuff like:

on: { 
  "A_EVENT, B_EVENT": "go here"
}

This can even be written like this:

on: { 
  [["A_EVENT", "B_EVENT"]]: "go here"
}

that is allowed because JS would just stringify the array put on the computed property and stringifying an array joins elements with a comma.

A different delimiter could be chosen, like whitespace, but then we couldn't leverage the computed property syntax. OTOH the computed property with an array doesn't play well with TS so it's not something that we should encourage, but it would work at a runtime level and a strict key can still be produced with something like:

on: {
  [`${A_EVENT_VARIABLE}, ${B_EVENT_VARIABLE}` as const]: 'go here'
}

I agree that this doesn't look that great and makes us "decode" the event descriptors from the property name. I'm not super attached to this idea but I think it's better not to allow this at all than to keep the array syntax in place. Having to copy-paste stuff can be a feature and not a limitation 😉 And note that the visual editor can simplify this - even if the target export syntax would involve some "duplicates" to be "emitted".

By removing this feature we don't loss expressiveness since there is a simple rewrite that can represents the same model.

💯

[davidkpiano]

Good to go @Andarist?

[Andarist]

this should be flagged with a TODO or something as it's a strong indication that things are wired weirdly here