Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[do not merge] Allow wildcard transitions to act as "error handlers" #2935

Closed
wants to merge 2 commits into from
Closed

[do not merge] Allow wildcard transitions to act as "error handlers" #2935

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ddba455
Allow wildcard transitions to act as "error handlers"
Andarist Jan 11, 2022
1afe0e6
Merge remote-tracking branch 'origin/next' into andarist/wildcards-fo…
Andarist Sep 19, 2023
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
4 changes: 1 addition & 3 deletions packages/core/src/interpreter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,9 +718,7 @@ export class Interpreter<

if (
_event.name.indexOf(actionTypes.errorPlatform) === 0 &&
!this.state.nextEvents.some(
(nextEvent) => nextEvent.indexOf(actionTypes.errorPlatform) === 0
)
!this.state.can(_event as any)
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

) {
throw (_event.data as any).data;
}
Expand Down
83 changes: 56 additions & 27 deletions packages/core/test/invoke.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1770,8 +1770,6 @@ describe('invoke', () => {
});

it('should call onError only on the state which has invoked failed service', () => {
let errorHandlersCalled = 0;

const errorMachine = Machine({
initial: 'start',
states: {
Expand All @@ -1784,44 +1782,48 @@ describe('invoke', () => {
type: 'parallel',
states: {
first: {
invoke: {
src: () => () => {
throw new Error('test');
},
onError: {
target: 'failed',
cond: () => {
errorHandlersCalled++;
return false;
initial: 'waiting',
Copy link
Member Author

Choose a reason for hiding this comment

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

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

states: {
waiting: {
invoke: {
src: () => () => {
throw new Error('test');
},
onError: {
target: 'failed'
}
}
}
},
failed: {}
}
},
second: {
invoke: {
src: () => () => {
// empty
},
onError: {
target: 'failed',
cond: () => {
errorHandlersCalled++;
return false;
initial: 'waiting',
states: {
waiting: {
invoke: {
src: () => () => {
// empty
},
onError: {
target: 'failed'
}
}
}
},
failed: {}
}
},
failed: {
type: 'final'
}
}
}
}
});

interpret(errorMachine).start().send('FETCH');
const service = interpret(errorMachine).start();
service.send('FETCH');

expect(errorHandlersCalled).toEqual(1);
expect(service.state.value).toEqual({
fetch: { first: 'failed', second: 'waiting' }
});
});

it('should be able to be stringified', () => {
Expand Down Expand Up @@ -2641,6 +2643,33 @@ describe('invoke', () => {
})
.start();
});

it('a wildcard transition should be able to receive an error event', (done) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

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 state onError transitions, 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeaaah, I think that xstate.error.* events should be treated in a special way. Wildcard transitions should not match them.

const machine = createMachine({
initial: 'fetching',
states: {
fetching: {
invoke: {
src: () => Promise.reject(new Error('test err')),
onDone: {
target: 'complete'
}
},
on: {
'*': {
actions: () => {
done();
}
}
}
},
complete: {
type: 'final'
}
}
});
interpret(machine).start();
});
});

it('invoke `src` should accept invoke source definition', (done) => {
Expand Down