statelyai / xstate

Actor-based state management & orchestration for complex app logic.
https://stately.ai/docs
MIT License
27.21k stars 1.26k forks source link

fix(core): errored initial snapshot throws sync w/o observers #5045

Closed boneskull closed 1 month ago

boneskull commented 3 months ago

This modifies the error-handling logic for when an error is thrown during the creation of an Actor's initial snapshot (during start()). If the actor has no observers, the error will now be thrown synchronously out of start() instead of to the global error handler.

Example use case:

const machine = createMachine({
  context: () => {
    throw new Error('egad!');
  }
});

const actor = createActor(machine);

try {
  await toPromise(actor.start());
} catch (err) {
  err.message === 'egad!' // true
}

Note that this does not impact child actors, but it might want to!

Fixes: #4928

changeset-bot[bot] commented 3 months ago

⚠️ No Changeset found

Latest commit: 0dabaa7be7e11b7567def5e1b747b4077cd8703f

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 no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

boneskull commented 3 months ago

For reference, the current workaround is this:

const machine = createMachine({
  context: () => {
    throw new Error('egad!');
  }
});
const actor = createActor(machine);

const p = toPromise(actor);
actor.start();
try {
  await p;
} catch (err) {
  err.message === 'egad!' // true
}
boneskull commented 3 months ago

I don't know if the new behavior is desirable to the maintainers, but I find it more ergonomic. Thought I would restart that conversation here!

boneskull commented 3 months ago

My main concern is that it is somewhat inconsistent, since if the example machine was spawned as a child actor, it would reach the global error handler.

...that being said, if using spawnChild(), there's really nowhere else for the error to go.

boneskull commented 3 months ago

This looks like a breaking change to me unless you feel this is actually a "bug fix" and not just a "revision of intent" 😜

Andarist commented 3 months ago

...that being said, if using spawnChild(), there's really nowhere else for the error to go.

Could you show the exact situation you are talking about here?

boneskull commented 3 months ago

...that being said, if using spawnChild(), there's really nowhere else for the error to go.

Could you show the exact situation you are talking about here?

@Andarist

something like this:

it('handled sync errors thrown when starting a child actor should be reported globally when spawned via spawnChild()', (done) => {
  const badMachine = createMachine({
    context: () => {
      throw new Error('handled_sync_error_in_actor_start');
    }
  });

  const machine = createMachine({
    entry: [
      spawnChild(badMachine)
    ]
  });

  const actorRef = createActor(machine);
  actorRef.start();
  installGlobalOnErrorHandler((ev) => {
    ev.preventDefault();
    expect(ev.error.message).toEqual('handled_sync_error_in_actor_start');
    done();
  });
});

If a xstate.error.actor.badMachine (or w/e) event handler in machine is the canonical way to trap this error, then I don't think changes to the above behavior are warranted (i.e. changing it so the error is thrown out of actor.start(), which I think is only possible because it's thrown synchronously?). It's just that there's no other reasonable way to catch it out of the root actor (e.g. if badMachine was the root actor), which is what this PR solves.