xstate icon indicating copy to clipboard operation
xstate copied to clipboard

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

Open boneskull opened this issue 1 year ago • 7 comments

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

boneskull avatar Aug 17 '24 23:08 boneskull

⚠️ 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

changeset-bot[bot] avatar Aug 17 '24 23:08 changeset-bot[bot]

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 avatar Aug 17 '24 23:08 boneskull

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 avatar Aug 17 '24 23:08 boneskull

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 avatar Aug 18 '24 20:08 boneskull

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

boneskull avatar Aug 19 '24 04:08 boneskull

...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 avatar Aug 19 '24 09:08 Andarist

...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.

boneskull avatar Aug 19 '24 20:08 boneskull