Visualize multiple actors

[davidkpiano]

CleanShot.2021-08-28.at.08.37.01.mp4

Stopped actors:

[changeset-bot]

⚠️ No Changeset found

Latest commit: 471ef72

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

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/statelyai/xstate-viz/HpXiToc6jZvXt7qguQUZnHWT5qZR
✅ Preview: https://xstate-viz-git-davidkpiano-sta-161-not-all-act-80fbd7-statelyai.vercel.app

[Andarist]

It IMHO would make slightly more sense to wrap the whole content of the ListItem in Link - if you are using the Link. That way you wouldn't have add cursor: pointer on this element and onClick would be placed on a slightly more appropriate (semantically) element

[Andarist]

This could be moved into the block responsible for handling the INTERPRET machine - we probably want to always call this logic then, so I'm not sure if lifting this up as an additional command that can be dispatched by the parent brings much to the table. And when analyzing this part of the code having them grouped would make it slightly easier to follow.

[davidkpiano]

Not sure about this; clearing the machines seems to be the responsibility of a 'STOP' event, not an 'INTERPRET' event. We can refactor later if we need to here.

[Andarist]

any particular reason behind this change? I don't see it immediately when reading through those changes and it seems that it either is not needed or it has fixed some race condition - but it's hard to tell without knowing more context behind this

[davidkpiano]

Without this change, when switching between actors, getElkGraph() is called when the previous graph is destroyed and the new graph isn't yet rendered; therefore, getRectMap(...) is useless and calling .width was throwing an error (can't read .width of undefined).

This was introduced to fix that consistent error.

[Andarist]

Ok, I see - we send the GRAPH_UPDATED in the useEffect but the last render that did happen was rendering based on the "success" state of the previous layout.

The used solution IMHO tries to fight the React and tries to wait, implicitly, on what should happen most of the time - but there are no conceptual guarantees that this will actually happen. For instance - we can click on another actor in the meantime and create a race condition.

It would be better if we could just avoid any chance of such things happening by making the synchronization of the DOM and Elk layouts more explicit, like for example here:
92d434a

I think it would be possible to avoid those duplicated effects but we'd have to implement derived state from props pattern and we'd have to avoid rendering based on state.matches('success'). So I've chosen a solution closer to XState, at the expense of some wasted rerenders in React.

[davidkpiano]

Let's have this as a refactor item

[mattpocock]

Should we be considering adding parent-child awareness to this PR? When visualising this code:

import { createMachine } from "xstate";

const childMachine = createMachine({
  initial: "idle",
  states: {
    idle: {},
  },
});

const parentMachine = createMachine({
  invoke: {
    src: childMachine,
  },
});

I see the child machine on screen first

[mattpocock]

The parser is ready for grabbing this kind of info:

statelyai/xstate-parser#3

[davidkpiano]

Should we be considering adding parent-child awareness to this PR?

The potential issue is when we have something like this:

const parentMachine = createMachine({
  initial: 'wait',
  states: {
    wait: {},
    active: {
      invoke: {
        src: childMachine,
      }
    }
  }
});

The childMachine should ideally not show up until it's invoked, but we can't prevent that, which is fine - we have a child machine which is not a child of the parent machine, and when we go to the active state, we will have a child machine which is a child of the parent machine. So we'll have 3 actors visualized.

This will probably cause confusion, but short of requiring interpret(...), we can't do much about it.

Also, childMachine can be invoked as part of anotherMachine, so it's not a 1-1 relationship.

[mattpocock]

@davidkpiano Did you see the XState parser PR above? I haven't got it set up for invokes yet but it certainly will be able to handle it

[mattpocock]

Should probably put this in a separate PR though, no need to hold up this one

[davidkpiano]

CI failing for an unrelated reason:

[linear]

STA-161 Not all actors being shown in the visualiser

https://www.loom.com/share/9a7fed533473432c93f175602b449dfb

[davidkpiano]

Ready for re-review

[farskid]

The code below is supposed to generate 4 actors but it spawns only 3.

import { createMachine, assign, spawn } from "xstate";
const machine = createMachine({
  id: "test machine",
  initial: "b",
  context: { ref: null },
  states: {
    a: {
      on: {
        GO: "b",
      },
    },
    b: {
      on: {
        GO: "a",
      },
    },
  },
});

const child = createMachine({
  id: "child",
  initial: "c",
  states: {
    c: {
      entry: assign({
        ref: () => spawn(child2),
      }),
    },
  },
});

const child2 = createMachine({
  id: "child2",
  initial: "d",
  states: {
    d: {},
  },
});

[farskid]

Holding this until the bug with the number of actors is fixed.

[davidkpiano]

The code below is supposed to generate 4 actors but it spawns only 3.

import { createMachine, assign, spawn } from "xstate";
const machine = createMachine({
  id: "test machine",
  initial: "b",
  context: { ref: null },
  states: {
    a: {
      on: {
        GO: "b",
      },
    },
    b: {
      on: {
        GO: "a",
      },
    },
  },
});

const child = createMachine({
  id: "child",
  initial: "c",
  states: {
    c: {
      entry: assign({
        ref: () => spawn(child2),
      }),
    },
  },
});

const child2 = createMachine({
  id: "child2",
  initial: "d",
  states: {
    d: {},
  },
});

The issue here is that context is missing:

const child = createMachine({
  id: "child",
  initial: "c",
+ context: {},
  states: {
    c: {
      entry: assign({
        ref: () => spawn(child2),
      }),
    },
  },
});

It works as expected with that fix ^

[farskid]

The code below is supposed to generate 4 actors but it spawns only 3.

import { createMachine, assign, spawn } from "xstate";
const machine = createMachine({
  id: "test machine",
  initial: "b",
  context: { ref: null },
  states: {
    a: {
      on: {
        GO: "b",
      },
    },
    b: {
      on: {
        GO: "a",
      },
    },
  },
});

const child = createMachine({
  id: "child",
  initial: "c",
  states: {
    c: {
      entry: assign({
        ref: () => spawn(child2),
      }),
    },
  },
});

const child2 = createMachine({
  id: "child2",
  initial: "d",
  states: {
    d: {},
  },
});

The issue here is that context is missing:

const child = createMachine({
  id: "child",
  initial: "c",
+ context: {},
  states: {
    c: {
      entry: assign({
        ref: () => spawn(child2),
      }),
    },
  },
});

It works as expected with that fix ^

ok. following the discussion we had on Discord, this will be the default in v5. Ship it.