[core] Add .onError() handler to service

[davidkpiano]

This PR adds the .onError(...) handler to services:

const service = interpret(machine)
  .onError(err => {
    // err is instanceof Error
    console.error(err.message);
  })
  .start();

This was necessary to test the behavior of tweaking reportUnhandledExceptionOnInvocation to check that originalError is actually an Error, since reject(anything) might not be an Error object, so err.stack would itself throw an error.

Still deciding if we want to add .onError((err, state) => { ... }) or other metadata 🤔

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: f2991f045ee0b37cd2df4e32510d52936d4cacbe

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes changesets to release 1 package

Name	Type
xstate	Patch

Click here to learn what changesets are, and how to add one.

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

[Andarist]

Would be great to define the role of this handler as mentioned here: https://github.com/davidkpiano/xstate/pull/1606#pullrequestreview-539334092 . We might change the semantics later but would be great to establish what this PR is currently aiming for.

[davidkpiano]

It would be great to define (in the docs) what's the exact purpose of onError. What it should do and what it can't do.

The purpose of onError is to be a mechanism for the parent to subscribe to errors, whether it's error.* events or thrown errors.

Is it useful only for reporting?

It's useful for reporting and reacting to errors.

Can it influence anyhow the machine's lifetime?

No.

[Andarist]

The purpose of onError is to be a mechanism for the parent to subscribe to errors, whether it's error.* events or thrown errors.

Is it supposed to handle only uncaught/unhandled errors/events or all? I think the best one would be the former - handled stuff would lead to noise. I haven't rechecked right now which is most likely based on the provided implementation - just trying to establish key goals of this PR without looking at the implementation for now.

Is it supposed to only receive errors that originated in the top-level machine or its invoked actors? Or should it (in the future perhaps) be responsible for receiving all uncaught errors from the whole "system" that it represents?

It's useful for reporting and reacting to errors.

How one would react to an error from it? I mean - what action could one take from there? Just kill the machine? Or maybe something else?

[davidkpiano]

The purpose of onError is to be a mechanism for the parent to subscribe to errors, whether it's error.* events or thrown errors.

Is it supposed to handle only uncaught/unhandled errors/events or all? I think the best one would be the former - handled stuff would lead to noise. I haven't rechecked right now which is most likely based on the provided implementation - just trying to establish key goals of this PR without looking at the implementation for now.

Is it supposed to only receive errors that originated in the top-level machine or its invoked actors? Or should it (in the future perhaps) be responsible for receiving all uncaught errors from the whole "system" that it represents?

To make it simple, onError is for all errors (uncaught and error.* event) that originate from the machine, since (according to the SCXML spec) they are the same thing:

Once the SCXML processor has begun executing a well-formed SCXML document, it must signal any errors that occur by raising SCXML events whose names begin with 'error.'.

https://www.w3.org/TR/scxml/#ErrorEvents

Actors don't send errors to the parent by default; that's what escalate() is for (or sendParent(...) if you want to do it manually), but these should work the same -- if an error is sent to the parent, onError listeners will be called with that error.

How one would react to an error from it? I mean - what action could one take from there? Just kill the machine? Or maybe something else?

That's up to the developer. Stop/restart the machine, log the error, etc.

[Andarist]

I would temporarily park this PR - until I post the error-related RFC, based on our Discord conversations. It has the potential of influencing the overall design of error handling but also this particular API.

[sebinsua]

@davidkpiano I noticed that errors thrown within .onTransition handlers are unhandled. Would this PR make it easier to catch these Service errors? Has the Error RFC been worked on yet? /cc @Andarist

The company I'm currently working for was considering using xstate within a backend Node server and using an onTransition handler to persist state changes into a database. In later versions of Node, unhandled rejections could crash the server. Also, ideally we'd want the server to be able to respond with a non-200 status code if there is an error and to not commit a database transaction if any part of it fails. This would imply that we would need to find a way to wait on the onTransition.

1. Is it safe to do database inserts within an onTransition handler? Do we just wrap our handler in a try-catch?
2. Or, should we instead move all database interactions into an invoked service that can explicitly move to a new state on success/fail?

Am I thinking about this in the wrong way?

[davidkpiano]

@davidkpiano I noticed that errors thrown within .onTransition handlers are unhandled. Would this PR make it easier to catch these Service errors? Has the Error RFC been worked on yet? /cc @Andarist

Yes, the error RFC has been recently been made internally, and we will surface it soon.

1. Is it safe to do database inserts within an onTransition handler? Do we just wrap our handler in a try-catch?

Yes you can; best to wrap in a try/catch 👍

2. Or, should we instead move all database interactions into an invoked service that can explicitly move to a new state on success/fail?

Currently, whatever is easiest for you all. Ideally, these interactions will be in an invoked service, but the end result is the same: the machine changing state is due to an event, whether those are events sent by a service or externally. Only a minor refactor is needed in the future if you're currently sending them externally and want to move to a service.

[Andarist]

@sebinsua https://github.com/statelyai/rfcs/pull/4

[Andarist]

This PR targets main and we don't plan to introduce any new major features to that release line. The error story is going to be revamped on the next branch and will be released in v5.