Further errors-related adjustments (#4492)

* Further errors-related adjuments * tweak tests and comments * add changeset * Update .changeset/olive-months-shave.md Co-authored-by: David Khourshid <[email protected]> --------- Co-authored-by: David Khourshid <[email protected]>
statelyai · Nov 29, 2023 · 63d9238 · 63d9238
1 parent 7916400
commit 63d9238
Show file tree

Hide file tree

Showing 16 changed files with 479 additions and 155 deletions.
diff --git a/.changeset/olive-months-shave.md b/.changeset/olive-months-shave.md
@@ -0,0 +1,5 @@
+---
+'xstate': major
+---
+
+All errors caught while executing the actor should now consistently include the error in its `snapshot.error` and should be reported to the closest `error` listener.
diff --git a/README.md b/README.md
@@ -82,7 +82,7 @@ const toggleMachine = createMachine({
     inactive: { on: { TOGGLE: 'active' } },
     active: {
       entry: assign({ count: ({ context }) => context.count + 1 }),
-      on: { TOGGLE: 'inactive' } 
+      on: { TOGGLE: 'inactive' }
     }
   }
 });

diff --git a/packages/core/src/StateMachine.ts b/packages/core/src/StateMachine.ts
@@ -2,7 +2,6 @@ import { assign } from './actions.ts';
 import { createInitEvent } from './eventUtils.ts';
 import { STATE_DELIMITER } from './constants.ts';
 import {
-  cloneMachineSnapshot,
   createMachineSnapshot,
   getPersistedState,
   MachineSnapshot
@@ -53,11 +52,7 @@ import type {
   HistoryValue,
   StateSchema
 } from './types.ts';
-import {
-  getAllOwnEventDescriptors,
-  isErrorActorEvent,
-  resolveReferencedActor
-} from './utils.ts';
+import { resolveReferencedActor } from './utils.ts';
 import { $$ACTOR_TYPE, createActor } from './interpreter.ts';
 import isDevelopment from '#is-development';
 
@@ -292,22 +287,7 @@ export class StateMachine<
     TOutput,
     TResolvedTypesMeta
   > {
-    // TODO: handle error events in a better way
-    if (
-      isErrorActorEvent(event) &&
-      !getAllOwnEventDescriptors(snapshot).some(
-        (nextEvent) => nextEvent === event.type
-      )
-    ) {
-      return cloneMachineSnapshot(snapshot, {
-        status: 'error',
-        error: event.data
-      });
-    }
-
-    const { state: nextState } = macrostep(snapshot, event, actorScope);
-
-    return nextState as typeof snapshot;
+    return macrostep(snapshot, event, actorScope).state as typeof snapshot;
   }
 
   /**

diff --git a/packages/core/src/actions/spawnChild.ts b/packages/core/src/actions/spawnChild.ts
@@ -105,12 +105,7 @@ function executeSpawn(
     if (actorRef._processingStatus === ProcessingStatus.Stopped) {
       return;
     }
-    try {
-      actorRef.start?.();
-    } catch (err) {
-      (actorScope.self as AnyActor).send(createErrorActorEvent(id, err));
-      return;
-    }
+    actorRef.start();
   });
 }
 

diff --git a/packages/core/src/interpreter.ts b/packages/core/src/interpreter.ts
@@ -206,11 +206,22 @@ export class Actor<TLogic extends AnyActorLogic>
   }
 
   private _initState(persistedState?: Snapshot<unknown>) {
-    this._state = persistedState
-      ? this.logic.restoreState
-        ? this.logic.restoreState(persistedState, this._actorScope)
-        : persistedState
-      : this.logic.getInitialState(this._actorScope, this.options?.input);
+    try {
+      this._state = persistedState
+        ? this.logic.restoreState
+          ? this.logic.restoreState(persistedState, this._actorScope)
+          : persistedState
+        : this.logic.getInitialState(this._actorScope, this.options?.input);
+    } catch (err) {
+      // if we get here then it means that we assign a value to this._state that is not of the correct type
+      // we can't get the true `TSnapshot & { status: 'error'; }`, it's impossible
+      // so right now this is a lie of sorts
+      this._state = {
+        status: 'error',
+        output: undefined,
+        error: err
+      } as any;
+    }
   }
 
   // array of functions to defer
@@ -224,19 +235,48 @@ export class Actor<TLogic extends AnyActorLogic>
     let deferredFn: (typeof this._deferred)[number] | undefined;
 
     while ((deferredFn = this._deferred.shift())) {
-      deferredFn();
-    }
-
-    for (const observer of this.observers) {
       try {
-        observer.next?.(snapshot);
+        deferredFn();
       } catch (err) {
-        reportUnhandledError(err);
+        // this error can only be caught when executing *initial* actions
+        // it's the only time when we call actions provided by the user through those deferreds
+        // when the actor is already running we always execute them synchronously while transitioning
+        // no "builtin deferred" should actually throw an error since they are either safe
+        // or the control flow is passed through the mailbox and errors should be caught by the `_process` used by the mailbox
+        this._deferred.length = 0;
+        this._state = {
+          ...(snapshot as any),
+          status: 'error',
+          error: err
+        };
       }
     }
 
     switch ((this._state as any).status) {
+      case 'active':
+        for (const observer of this.observers) {
+          try {
+            observer.next?.(snapshot);
+          } catch (err) {
+            reportUnhandledError(err);
+          }
+        }
+        break;
       case 'done':
+        // next observers are meant to be notified about done snapshots
+        // this can be seen as something that is different from how observable work
+        // but with observables `complete` callback is called without any arguments
+        // it's more ergonomic for XState to treat a done snapshot as a "next" value
+        // and the completion event as something that is separate,
+        // something that merely follows emitting that done snapshot
+        for (const observer of this.observers) {
+          try {
+            observer.next?.(snapshot);
+          } catch (err) {
+            reportUnhandledError(err);
+          }
+        }
+
         this._stopProcedure();
         this._complete();
         this._doneEvent = createDoneActorEvent(
@@ -249,15 +289,7 @@ export class Actor<TLogic extends AnyActorLogic>
 
         break;
       case 'error':
-        this._stopProcedure();
         this._error((this._state as any).error);
-        if (this._parent) {
-          this.system._relay(
-            this,
-            this._parent,
-            createErrorActorEvent(this.id, (this._state as any).error)
-          );
-        }
         break;
     }
     this.system._sendInspectionEvent({
@@ -365,7 +397,7 @@ export class Actor<TLogic extends AnyActorLogic>
       this.subscribe({
         next: (snapshot: Snapshot<unknown>) => {
           if (snapshot.status === 'active') {
-            this._parent!.send({
+            this.system._relay(this, this._parent!, {
               type: `xstate.snapshot.${this.id}`,
               snapshot
             });
@@ -401,19 +433,23 @@ export class Actor<TLogic extends AnyActorLogic>
           this._state,
           initEvent as unknown as EventFromLogic<TLogic>
         );
-      // fallthrough
-      case 'error':
         // TODO: rethink cleanup of observers, mailbox, etc
         return this;
+      case 'error':
+        this._error((this._state as any).error);
+        return this;
     }
 
     if (this.logic.start) {
       try {
         this.logic.start(this._state, this._actorScope);
       } catch (err) {
-        this._stopProcedure();
+        this._state = {
+          ...(this._state as any),
+          status: 'error',
+          error: err
+        };
         this._error(err);
-        this._parent?.send(createErrorActorEvent(this.id, err));
         return this;
       }
     }
@@ -433,7 +469,6 @@ export class Actor<TLogic extends AnyActorLogic>
   }
 
   private _process(event: EventFromLogic<TLogic>) {
-    // TODO: reexamine what happens when an action (or a guard or smth) throws
     let nextState;
     let caughtError;
     try {
@@ -446,9 +481,12 @@ export class Actor<TLogic extends AnyActorLogic>
     if (caughtError) {
       const { err } = caughtError;
 
-      this._stopProcedure();
+      this._state = {
+        ...(this._state as any),
+        status: 'error',
+        error: err
+      };
       this._error(err);
-      this._parent?.send(createErrorActorEvent(this.id, err));
       return;
     }
 
@@ -492,7 +530,7 @@ export class Actor<TLogic extends AnyActorLogic>
     }
     this.observers.clear();
   }
-  private _error(err: unknown): void {
+  private _reportError(err: unknown): void {
     if (!this.observers.size) {
       if (!this._parent) {
         reportUnhandledError(err);
@@ -515,6 +553,22 @@ export class Actor<TLogic extends AnyActorLogic>
       reportUnhandledError(err);
     }
   }
+  private _error(err: unknown): void {
+    this._stopProcedure();
+    this._reportError(err);
+    if (this._parent) {
+      this.system._relay(
+        this,
+        this._parent,
+        createErrorActorEvent(this.id, err)
+      );
+    }
+  }
+  // TODO: atm children don't belong entirely to the actor so
+  // in a way - it's not even super aware of them
+  // so we can't stop them from here but we really should!
+  // right now, they are being stopped within the machine's transition
+  // but that could throw and leave us with "orphaned" active actors
   private _stopProcedure(): this {
     if (this._processingStatus !== ProcessingStatus.Running) {
       // Actor already stopped; do nothing

diff --git a/packages/core/src/spawn.ts b/packages/core/src/spawn.ts
@@ -111,12 +111,7 @@ export function createSpawner(
       if (actorRef._processingStatus === ProcessingStatus.Stopped) {
         return;
       }
-      try {
-        actorRef.start?.();
-      } catch (err) {
-        actorScope.self.send(createErrorActorEvent(actorRef.id, err));
-        return;
-      }
+      actorRef.start();
     });
     return actorRef;
   };

diff --git a/packages/core/src/stateUtils.ts b/packages/core/src/stateUtils.ts
@@ -45,7 +45,8 @@ import {
   normalizeTarget,
   toArray,
   toStatePath,
-  toTransitionConfigArray
+  toTransitionConfigArray,
+  isErrorActorEvent
 } from './utils.ts';
 import { ProcessingStatus } from './interpreter.ts';
 
@@ -1643,7 +1644,25 @@ export function macrostep(
   // Assume the state is at rest (no raised events)
   // Determine the next state based on the next microstep
   if (nextEvent.type !== XSTATE_INIT) {
-    const transitions = selectTransitions(nextEvent, nextState);
+    const currentEvent = nextEvent;
+    const isErr = isErrorActorEvent(currentEvent);
+
+    const transitions = selectTransitions(currentEvent, nextState);
+
+    if (isErr && !transitions.length) {
+      // TODO: we should likely only allow transitions selected by very explicit descriptors
+      // `*` shouldn't be matched, likely `xstate.error.*` shouldnt be either
+      // similarly `xstate.error.actor.*` and `xstate.error.actor.todo.*` have to be considered too
+      nextState = cloneMachineSnapshot<typeof state>(state, {
+        status: 'error',
+        error: currentEvent.data
+      });
+      states.push(nextState);
+      return {
+        state: nextState,
+        microstates: states
+      };
+    }
     nextState = microstep(
       transitions,
       state,

diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts
@@ -1918,8 +1918,7 @@ export interface ActorRef<
   /** @internal */
   _send: (event: TEvent) => void;
   send: (event: TEvent) => void;
-  // TODO: should this be optional?
-  start?: () => void;
+  start: () => void;
   getSnapshot: () => TSnapshot;
   getPersistedState: () => Snapshot<unknown>;
   stop: () => void;

diff --git a/packages/core/test/actions.test.ts b/packages/core/test/actions.test.ts
@@ -3185,7 +3185,7 @@ describe('sendTo', () => {
     service.send({ type: 'EVENT', value: 'foo' });
   });
 
-  it('should throw if given a string', () => {
+  it('should error if given a string', () => {
     const machine = createMachine({
       invoke: {
         id: 'child',
@@ -3194,11 +3194,21 @@ describe('sendTo', () => {
       entry: sendTo('child', 'a string')
     });
 
-    expect(() => {
-      createActor(machine).start();
-    }).toThrowErrorMatchingInlineSnapshot(
-      `"Only event objects may be used with sendTo; use sendTo({ type: "a string" }) instead"`
-    );
+    const errorSpy = jest.fn();
+
+    const actorRef = createActor(machine);
+    actorRef.subscribe({
+      error: errorSpy
+    });
+    actorRef.start();
+
+    expect(errorSpy).toMatchMockCallsInlineSnapshot(`
+      [
+        [
+          [Error: Only event objects may be used with sendTo; use sendTo({ type: "a string" }) instead],
+        ],
+      ]
+    `);
   });
 });
 
@@ -3410,19 +3420,29 @@ describe('raise', () => {
     }, 10);
   });
 
-  it('should throw if given a string', () => {
+  it('should error if given a string', () => {
     const machine = createMachine({
       entry: raise(
         // @ts-ignore
         'a string'
       )
     });
 
-    expect(() => {
-      createActor(machine).start();
-    }).toThrowErrorMatchingInlineSnapshot(
-      `"Only event objects may be used with raise; use raise({ type: "a string" }) instead"`
-    );
+    const errorSpy = jest.fn();
+
+    const actorRef = createActor(machine);
+    actorRef.subscribe({
+      error: errorSpy
+    });
+    actorRef.start();
+
+    expect(errorSpy).toMatchMockCallsInlineSnapshot(`
+      [
+        [
+          [Error: Only event objects may be used with raise; use raise({ type: "a string" }) instead],
+        ],
+      ]
+    `);
   });
 });