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

[core] feat(interpreter): allow to provide errorListeners #2841

Closed
wants to merge 17 commits into from
Closed

[core] feat(interpreter): allow to provide errorListeners #2841

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
123ae97
feat(interpreter): allow to provide errorListeners
roggervalf Nov 28, 2021
f5027b0
chore: add a changeset
roggervalf Nov 28, 2021
c0a2a55
refactor(interpreter): handle more error cases
roggervalf Nov 28, 2021
fbc8f50
chore(changeset): change major for minor
roggervalf Nov 28, 2021
cf32294
refactor: handle case where there are child machines
roggervalf Dec 2, 2021
baa772f
refactor: handle case where there are grandchild machines
roggervalf Dec 2, 2021
5b1372e
refactor: just check the type error event
roggervalf Dec 3, 2021
a49c586
chore: delete extra parameter in devTools send call
roggervalf Dec 3, 2021
77f034b
refactor: consider send all error events to error listerner
roggervalf Dec 11, 2021
b5b4dd0
chore: use createMachine
roggervalf Dec 11, 2021
42a307f
test: fix test case
roggervalf Dec 11, 2021
f4d1d1b
refactor: send error as error.platform
roggervalf Dec 12, 2021
610686a
chore: merge branch 'main' into feat-errorListerners
roggervalf Dec 17, 2021
52037b0
refactor: handle error event
roggervalf Jan 11, 2022
6d90543
Merge branch 'main' into feat-errorListerners
roggervalf Jan 11, 2022
243eebe
refactor: respect strict mode
roggervalf Jan 11, 2022
865f2ce
chore: send state as payload in errorEvent
roggervalf Jan 11, 2022
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
5 changes: 5 additions & 0 deletions .changeset/famous-icons-flash.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'xstate': minor
---

allow to provide errorListeners into interpreter
82 changes: 72 additions & 10 deletions packages/core/src/interpreter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ export class Interpreter<
private contextListeners: Set<ContextListener<TContext>> = new Set();
private stopListeners: Set<Listener> = new Set();
private doneListeners: Set<EventListener> = new Set();
private errorListeners: Set<EventListener> = new Set();
private eventListeners: Set<EventListener> = new Set();
private sendListeners: Set<EventListener> = new Set();
private logger: (...args: any[]) => void;
Expand Down Expand Up @@ -313,6 +314,15 @@ export class Interpreter<
this.stop();
}
}

private sendError(errorEvent: Event<TEvent> | SCXML.Event<TEvent>): void {
if (this.errorListeners.size) {
for (const listener of this.errorListeners) {
listener(errorEvent as EventObject);
}
}
}

/*
* Adds a listener that is notified whenever a state transition happens. The listener is called with
* the next state and the event object that caused the state transition.
Expand Down Expand Up @@ -343,7 +353,7 @@ export class Interpreter<
nextListenerOrObserver?:
| ((state: State<TContext, TEvent, any, TTypestate>) => void)
| Observer<State<TContext, TEvent, any, TTypestate>>,
_?: (error: any) => void, // TODO: error listener
errorListener?: (error: any) => void,
completeListener?: () => void
): Subscription {
if (!nextListenerOrObserver) {
Expand All @@ -352,6 +362,7 @@ export class Interpreter<

let listener: (state: State<TContext, TEvent, any, TTypestate>) => void;
let resolvedCompleteListener = completeListener;
let resolvedErrorListener = errorListener;

if (typeof nextListenerOrObserver === 'function') {
listener = nextListenerOrObserver;
Expand All @@ -373,11 +384,17 @@ export class Interpreter<
this.onDone(resolvedCompleteListener);
}

if (resolvedErrorListener) {
this.onError(resolvedErrorListener);
}

return {
unsubscribe: () => {
listener && this.listeners.delete(listener);
resolvedCompleteListener &&
this.doneListeners.delete(resolvedCompleteListener);
resolvedErrorListener &&
this.errorListeners.delete(resolvedErrorListener);
}
};
}
Expand Down Expand Up @@ -432,6 +449,16 @@ export class Interpreter<
this.doneListeners.add(listener);
return this;
}
/**
* Adds a state listener that is notified when the statechart has reached an error.
* @param listener The state listener
*/
public onError(
listener: EventListener<DoneEvent>
): Interpreter<TContext, TStateSchema, TEvent, TTypestate> {
this.errorListeners.add(listener);
return this;
}
/**
* Removes a listener.
* @param listener The listener to remove
Expand All @@ -444,6 +471,7 @@ export class Interpreter<
this.sendListeners.delete(listener);
this.stopListeners.delete(listener);
this.doneListeners.delete(listener);
this.errorListeners.delete(listener);
this.contextListeners.delete(listener);
return this;
}
Expand Down Expand Up @@ -510,6 +538,9 @@ export class Interpreter<
for (const listener of this.doneListeners) {
this.doneListeners.delete(listener);
}
for (const listener of this.errorListeners) {
this.errorListeners.delete(listener);
}

if (!this.initialized) {
// Interpreter already stopped; do nothing
Expand Down Expand Up @@ -552,13 +583,22 @@ export class Interpreter<
*/
public send = (
event: SingleOrArray<Event<TEvent>> | SCXML.Event<TEvent>,
payload?: EventData
payload?: EventData,
sendError = false
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this; errors should either be unhandled error.* events or "natural" errors thrown from execution of the machine.

Copy link
Author

Choose a reason for hiding this comment

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

thank you @davidkpiano I just see your message, in my last commit I was replacing this by checking the type in the event, is that ok or should I do in a different way?

): State<TContext, TEvent, TStateSchema, TTypestate> => {
if (isArray(event)) {
this.batch(event);
return this.state;
}

if (sendError) {
if (this.parent) {
this.parent.sendError(event as any);
} else {
this.sendError(event as any);
Copy link
Member

Choose a reason for hiding this comment

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

This one sounds like it's going to send this event somewhere (like some kind of an actor or something), but it's more like notifying current subscribers.

I also do not (overall) liking how this might make more errors to be swallowed. In general, the error story in XState is currently under-specified so it's somewhat hard to answer "how this should behave now?". I would love to hear out your thoughts about this RFC: statelyai/rfcs#4 . Going through that could also show you my perspective better.

Copy link
Author

Choose a reason for hiding this comment

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

Looks pretty nice, could you please guide me on the proper changes when you get some chance?

}
}

const _event = toSCXMLEvent(toEventObject(event as Event<TEvent>, payload));

if (this.status === InterpreterStatus.Stopped) {
Expand Down Expand Up @@ -788,10 +828,14 @@ export class Interpreter<
});
} catch (err) {
if (this.parent) {
this.parent.send({
type: 'xstate.error',
data: err
} as EventObject);
this.parent.send(
{
type: 'xstate.error',
data: err
} as EventObject,
undefined,
true
);
}

throw err;
Expand Down Expand Up @@ -1053,10 +1097,24 @@ export class Interpreter<
// Send "error.platform.id" to this (parent).
this.send(toSCXMLEvent(errorEvent as any, { origin: id }));
} catch (error) {
reportUnhandledExceptionOnInvocation(errorData, error, id);
if (this.parent) {
this.parent.send(
{
type: 'xstate.error',
data: error
} as EventObject,
undefined,
true
);
} else if (!this.errorListeners.size) {
reportUnhandledExceptionOnInvocation(errorData, error, id);
}
if (this.devTools) {
this.devTools.send(errorEvent, this.state);
this.devTools.send(errorEvent, this.state, true);
}

this.sendError(errorEvent);

if (this.machine.strict) {
// it would be better to always stop the state machine if unhandled
// exception/promise rejection happens but because we don't want to
Expand Down Expand Up @@ -1134,7 +1192,7 @@ export class Interpreter<
receivers.add(newListener);
});
} catch (err) {
this.send(error(id, err) as any);
this.send(error(id, err) as any, undefined, true);
}

if (isPromiseLike(callbackStop)) {
Expand Down Expand Up @@ -1184,7 +1242,11 @@ export class Interpreter<
},
(err) => {
this.removeChild(id);
this.send(toSCXMLEvent(error(id, err) as any, { origin: id }));
this.send(
toSCXMLEvent(error(id, err) as any, { origin: id }),
undefined,
true
);
},
() => {
this.removeChild(id);
Expand Down
166 changes: 166 additions & 0 deletions packages/core/test/interpreter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1689,6 +1689,172 @@ Event: {\\"type\\":\\"SOME_EVENT\\"}"
);
});

it('should be subscribable to errorListener', (done) => {
const failureMachine = Machine<typeof context>(
{
id: 'interval',
context,
initial: 'active',
states: {
active: {
after: {
10: {
target: 'failure'
}
}
},
failure: {
invoke: {
src: 'failure'
}
}
}
},
{
services: {
failure: async () => {
throw new Error('error');
}
}
}
);

const intervalService = interpret(failureMachine).start();

expect(isObservable(intervalService)).toBeTruthy();

intervalService.subscribe(
() => {},
(error) => {
expect(error.data).toBeInstanceOf(Error);
done();
}
);
});

it('should handle child errors with errorListener', (done) => {
const failureMachine = Machine<typeof context>(
{
id: 'failure',
context,
initial: 'active',
states: {
active: {
after: {
100: {
target: 'failure'
}
}
},
failure: {
invoke: {
src: 'failure'
}
}
}
},
{
services: {
failure: async () => {
throw new Error('error');
}
}
}
);

const parentMachine = Machine({
initial: 'foo',
states: {
foo: {
invoke: {
id: 'child',
src: failureMachine
}
}
}
});

const intervalService = interpret(parentMachine).start();

expect(isObservable(intervalService)).toBeTruthy();

intervalService.subscribe(
() => {},
(error) => {
expect(error.data).toBeInstanceOf(Error);
done();
}
);
});

it('should handle grandchild errors with errorListener', (done) => {
const failureMachine = Machine<typeof context>(
{
id: 'failure',
context,
initial: 'active',
states: {
active: {
after: {
100: {
target: 'failure'
}
}
},
failure: {
invoke: {
src: 'failure'
}
}
}
},
{
services: {
failure: async () => {
throw new Error('error');
}
}
}
);

const parentMachine = Machine({
initial: 'foo',
states: {
foo: {
invoke: {
id: 'child',
src: failureMachine
}
}
}
});

const grandparentMachine = Machine({
id: 'grandparent',
initial: 'bar',
states: {
bar: {
invoke: {
id: 'parent',
src: parentMachine
}
}
}
});

const intervalService = interpret(grandparentMachine).start();

expect(isObservable(intervalService)).toBeTruthy();

intervalService.subscribe(
() => {},
(error) => {
expect(error.data).toBeInstanceOf(Error);
done();
}
);
});

it('should be interoperable with RxJS, etc. via Symbol.observable', (done) => {
let count = 0;
const intervalService = interpret(intervalMachine).start();
Expand Down