[do not merge] Allow wildcard transitions to act as "error handlers" #2935
Conversation
|
CodeSee Review Map:Review in an interactive map View more CodeSee Maps Legend |
packages/core/test/invoke.test.ts
Outdated
| cond: () => { | ||
| errorHandlersCalled++; | ||
| return false; | ||
| initial: 'waiting', |
There was a problem hiding this comment.
this test was weirdly written so I've refactored it in the process, this should land on the main branch regardless of this PR - I will create a branch with just this simple adjustment later on
| @@ -2641,6 +2643,33 @@ describe('invoke', () => { | |||
| }) | |||
| .start(); | |||
| }); | |||
|
|
|||
| it('a wildcard transition should be able to receive an error event', (done) => { | |||
There was a problem hiding this comment.
This is a new test case that only passes with the introduced implementation change.
It initially made sense to me to introduce this change in a vein that "error transitions are just transitions". However, after thinking about it:
- this is somewhat implicit and might not meet people's expectations
- it hurts the types - errors in JS are already hard to type as you can throw "just anything". We could, maybe, just add a special event like
{ type: 'error.state', data: unknown }to the list of known events that would have to be handled by users (and that would be generated by the typegen and so on) but initially I've thought about "scoping" error event names to make the implementation of the ideas in RFC: Errors in XState rfcs#4 easier (we need to deconflict per stateonErrortransitions, this could be done with closure-based guards but that's suboptimal for visualization and all). If we go with a solution that is based on scoping those events then it, IMHO, becomes impractical to add a, potentially huge, list of possible error event names to all wildcard transitions. OTOH wildcard transitions should be discouraged so maybe this is not that big of a problem.
TLDR. it should be decided how this should behave as part of the statelyai/rfcs#4 . Note that the first argument still stands, it's not that obvious that * would "turn off" all of the assumed "error semantics", so it might make sense to special case treatment of error.* events in this regard either way.
Thoughts?
There was a problem hiding this comment.
Yeaaah, I think that xstate.error.* events should be treated in a special way. Wildcard transitions should not match them.
packages/core/src/interpreter.ts
Outdated
| !this.state.nextEvents.some( | ||
| (nextEvent) => nextEvent.indexOf(actionTypes.errorPlatform) === 0 | ||
| ) | ||
| !this.state.can(_event as any) |
There was a problem hiding this comment.
Note that both the current logic (the one on the main branch) and the proposed logic in this PR sound incorrect to me, regardless of the decisions that have to be made in response to the other comment.
Incorrect scenarios are really edge-case-y though and applicable more in v5 context than in v4. This whole if statement might be also dropped entirely as part of the work resulting from The Errors RFC so it's not worth addressing this now.
With the current logic, based on nextEvents, this won't quite work correctly when we consider parallel regions because a single onError in one of the regions "turns off" this if statement for another parallel region.
The proposed logic feels incorrect because we can create such structure:
on: {
[errorDescriptor]: { /* do smth */ },
},
states: {
a: {
invoke: {
src: 'fetch',
onError: undefined
}
}
}As we might see here - the inner error handler is forbidden but the outer one is not (the latter doesn't quite matter but maybe other edge cases can be created using such setup). This would return false from state.can(errorEvent) when in the a state but the onError: undefined looks explicit enough to count this as an "explicit" error handler. It feels that it would make sense to swallow errors with such a forbidden onError - note that it's not obvious how to express "error swallowing" differently. The only thing that comes to my mind is this:
onError: { actions: noop }And I wouldn't count this as intuitive.
There was a problem hiding this comment.
I added v5 essential label to this PR just to remember to incorporate thoughts from this comment above. The PR itself has to be redone.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 1afe0e6:
|
…r-errors # Conflicts: # packages/core/src/interpreter.ts # packages/core/test/invoke.test.ts
Andarist
force-pushed
the
andarist/wildcards-for-errors
branch
from
8d402c2
to
1afe0e6
Compare
|
Let's prefer |
|
Decision:
|
|
🧹 We should have |
While investigating #2919 I've noticed some things and played around with minor code adjustments.