Interpreter.start(compoundState) does not execute activities. Is this expected behavior?

womcauliff commented 5 years ago

Bug or feature request?

Bug, unless this is expected behavior.

(Bug) Expected result:

When I start the interpreter with a compound state node, I assumed it would also start its activities.

(Bug) Actual result:

Starting the interpreter with atomic state node does start its activities, but starting the interpreter with compound state node does not.

(Bug) Potential fix:

(Assuming what I'm seeing is not expected behavior.)

I took a look at interpreter source code. When the interpreter is started with a State node, it re-executes each action in the actions array, and if there is an action of type: "xstate.start" and an activity property, then the activity will be started. But in the case where the activity is already in progress, and the xstate.start action is already gone, then the activity is not started.

I see there is an activities array in the State object, where each activity name maps to true when it is running. A potential fix for the interpreter.start() method might be for it to also iterate over the activities array, looking for the activities that are true, and then calling the associated activity function. However, it should avoid calling activity function if there is already an xstate.start action leading to the start of the activity.

Link to reproduction or proof-of-concept:

Starting interpreter with rehydrated compound state https://codesandbox.io/s/ollk3xz4z6
- Navigate to state active.B, and refresh the browser. Here the blink activity will not be started.
Starting interpreter with rehydrated atomic state https://codesandbox.io/s/3xmp6ymv86
- Navigate to state active, and refresh the browser. Here, the blink activity will be started.

davidkpiano commented 5 years ago

Thanks for thinking through the potential fix. Saves me a ton of time 😄

You're right, this might not be expected behavior - activities that are assumed to have already started should be restarted when rehydrating state. I'll fix this.

In the future, there might be a restartActivities: false flag for the interpreter in case the intention is for the activities not to be restarted.

mogsie commented 5 years ago

Speaking generally about statecharts, when an activity has been started, it's started. I think it's wrong to ask the statechart to restart the activities that were stopped when the page refreshed. When rehydrating a state, the state machine sort of knows which activities are running (it's essentially a function of which state the machine is in) — it's even stored in the 'acitivites' hash, as you mention. I would advise against automatically restarting any activities that the machine thinks should be started (upon rehydration).

Consider the case where you didn't refresh the page, but perhaps removed a component and it got re-instantiated to the component tree, and wants to continue its statechart from where it left off (like your example), but where the activity actually reflected a long running process on a server (e.g. backed by a Web Socket). In this scenario, it would be wrong to tell the server to start an activity that presumably already was started when the machine / component last was running.

I think it would be better to fix this by changing the handling of restoredStateDef and ensure that any activity that was previously started, is restarted:

I would keep a reference to the activities

    this.config = {
      activities: {
        blink: this.createBlinkerActivity.bind(this)
      }
    };
    this.machine = toggleMachine.withConfig(this.config);

then later, when loading the state from storage, start those activites that were saved as being started.

      const restoredState = State.create(restoredStateDef);
      if (restoredState.activities) {
        Object.keys(restoredState.activities)
          .filter(activity => restoredState.activities[activity])  // only start the ones that are 'true'
          .filter(activity => !!this.config.activities[activity])  // avoid 'undefined is not a function'
          .forEach(activity => this.config.activities[activity]());
      }
      this.service.start(restoredState);

It could be that the interpreter should provide this as a helper function, but it should be something you "opt in to" and only with the knowledge that the activity is really transient in nature.

mogsie commented 5 years ago

@davidkpiano and I were looking at this independently, but came to the same conclusion: that this (restarting activities automatically) isn't such a good idea. David mentioned providing a mechanism to start activities in a way that the machine can still be in control of the stopping of those activities:

const restoredState = ...
Object.keys(restoredState.activities).forEach(activityKey => {
  // filter first, and then...
  restoredState.actions.push(start(activityKey));
});

service.start(restoredState);

This fixes my bad take at it (it keeps blinking if you go to the Off state after rehydration 🙈), but still allows you to start the activities that you know need to be started.

davidkpiano commented 5 years ago

Docs added:

Restarting Activities

When restoring a persisted state, activities that were previously running are not restarted by default. This is to prevent undesirable and/or unexpected behavior. However, activities can be manually started by adding start(...) actions to the persisted state before restarting a service:

import { State, actions } from 'xstate';

// ...

const restoredState = State.create(somePersistedStateJSON);

// Select activities to be restarted
Object.keys(restoredState.activities).forEach(activityKey => {
  if (restoredState.activities[activityKey]) {
    // Filter activities, and then add the start() action to the restored state
    restoredState.actions.push(actions.start(activityKey));
  }
});

// This will start someService
// with the activities restarted.
someService.start(restoredState);

womcauliff commented 5 years ago

@davidkpiano @mogsie really good points -- in particular I hadn't considered the case of activity being started on server-side. Thanks for the guidance on how I can account for activity restart and the docs update!

statelyai / xstate