[@xstate/react] Add asEffect and asLayoutEffect

[davidkpiano]

This PR adds support for actions that should be executed in useEffect and useLayoutEffect by adding two helper functions:

• asEffect - queues the action to be executed in useEffect
• asLayoutEffect - queues the action to be executed in useLayoutEffect

const machine = createMachine({
  initial: 'focused',
  states: {
    focused: {
      entry: 'focus'
    }
  }
});

const Input = () => {
  const inputRef = useRef(null);
  const [state, send] = useMachine(machine, {
    actions: {
      focus: asEffect(() => {
        inputRef.current && inputRef.current.focus();
      })
    }
  });

  return <input ref={inputRef} />
}

[changeset-bot]

💥 No Changeset

Latest commit: 7edf4c1

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, 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

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7edf4c1:

Sandbox	Source
exciting-grass-tollj	Configuration
zealous-kirch-51pq1	Configuration

[Andarist]

I've left a few concerns but overall I really like the direction of this PR and the proposed API 👍

[Andarist]

This wont quite work OK with SSR - people will get warnings about useLayoutEffect being used on server. We should use „isomorphic layout effect” - basically conditionally use layout or regular effect based on the environment. Its what other libraries do - for example react-redux. I was just preparing a package for this earlier this week (which is just reexporting correct thing, but handles browser, ssr, react-native correctly). We could reuse it or inline its content here - i could handle this once this PR lands as i already know how to do this „best”

[davidkpiano]

That would be great. I'll leave this open as a note to do that.

[Andarist]

nit: Hard to tell without actually measuring this, but I suppose this might even decrease perf a little bit, you need to allocate this temp array on each render, and do the comparison of old and new inputs. I suspect that always calling an effect and doing a simple .length check in it might be better overall.

[Andarist]

I believe that we should move existing queued actions to a local variable and assign a fresh empty array to this ref. If the execution of a queued effect leads to queuing an effect then when should it happen? Within the same flushing operation? Or should it schedule it for another flush within the upcoming render that is going to happen anyway because the state had to be set? I would be in favor of scheduling it for another flush because one might assume that a scheduled effect happens when things (most notably DOM elements) are ready to be used but a particular element might not exist if we just append to the existing flushing operation (imagine asEffect leading [assign, asEffect] where this assign is causing a new element to be rendered)

[davidkpiano]

If the execution of a queued effect leads to queuing an effect then when should it happen?

This will never happen (immediately), as the only way an action can possibly do this is by sending an event to the service, which might queue new actions only after these actions have been flushed.

We can add a test for this if I'm wrong, though.

[Andarist]

While doing asEffect(() => send(FOO)) is definitely a weird thing to do, so it's kinda unrealistic, it can be somewhere deep down in the stack. But flattening more complex case which could happen in a real app to this simplistic asEffect(() => send(FOO)) leads me to believe that an effect action could get in fact be executed in the middle of this flushing.

After all, this is being flushed whenever React decides to do so, so XState is not aware of this, it is happening outside of its internal scheduler which means that first send will execute immediately which in turn will cause onTransition to be called synchronously (still while flushing) and this might lead to a new effect action to be queued at the end of the currently flushed items list.

[davidkpiano]

Can we add a test for this? I don't believe this potential edge-case is a showstopper.

[Andarist]

sure, gonna add a test for this

[Andarist]

I'm worried that those currently are called with "settled" arguments whereas regular actions are called with the current arguments from the time when a transition was selected. This makes those different from other builtin actions.

It would be IMHO preferable to have those behaving more like the rest of builtin actions. A really quick idea of how we could solve this:

Introduce a new capture action (tentative name). It could then be "resolved" by resolveActions and actually executed by exec in the Interpreter class. In the future, we could consider "capturing" all custom actions to make them behave closer to the builtin ones in this regard. I think this is quite important because it seems quite expected to have FOO event's type here at all times:

on: { FOO: { actions: (ctx, event) => {} } }

[davidkpiano]

This is as intended. When the actions are called, it is always assumed that they are called with the data (context and event) that occurred when the action is returned as part of the state. Otherwise, this can lead to non-deterministic behavior (the same action doing potentially different things depending on when it is called)

[davidkpiano]

To clarify w/r/t determinism: an action executed immediately should behave exactly the same as that same action executed e.g., 20ms later. This is even true in React, in which effects might be executed with stale data still.

[Andarist]

Hm, not sure if we are on the same page here, so maybe let me rephrase my concerns with an example:

FOO: {
  actions: [
    assign((ctx1, ev1) => {},
    (ctx2, ev2) => {}),
    raise(/**/)
  ]
}

Given this example ev1 !== ev2 because the second action, the one which is not a builtin one, gets executed (and arguments are provided to it) when the machine "settles", whereas for builtin actions we "capture" current (from the moment when a transition has happened) context/event. In a similar fashion the problem doesn't only impact event argument, but also context - for assign (in v5) this would refer to a context available at the time of selecting a transition whereas for non-core action this receives computed context after things settle (so if for example assign follows non-core action then that non-core action receives "updated" context, but imho it shouldn't).

IMHO the difference described above leads to non-determinism and makes things harder to reason about.

[davidkpiano]

Actually, this is a non-issue. I think you're confusing how the actions get resolved with how the actions get executed.

In both V4 and V5, the same arguments will be passed to the resolved action: state.context, state.event, { state, ... }. How the resolved action gets determined is the issue. In both V4 and V5, custom actions are not called with intermediate context, although they should be. (Built-in actions like send(), log(), choose(), etc. are, though).

This needs to be fixed in xstate (and is a breaking change, so V5), not in here.

[Andarist]

custom actions are not called with intermediate context, although they should be. (Built-in actions like send(), log(), choose(), etc. are, though).

Right, but as those new actions here are sort-of builtins I think they should share the semantics with builtin actions, but right now they share it with custom ones.

This needs to be fixed in xstate (and is a breaking change, so V5), not in here.

Agreed as to the custom ones.

I think you're confusing how the actions get resolved with how the actions get executed.

Yes, definitely part of my confusion. But if we in v5 plan to fix this also for custom actions then there won't be much a difference between what arguments are given to both builtin and custom actions as the resolving step would capture arguments and executing would just call what with what got captured - there wouldn't be any different set of arguments given when executing.

[Andarist]

Right now this won't work at all for invoked/spawned machines. I have no good idea on how to solve this just yet in a way that would not be involving submachines somehow reaching to the "root" of the created machine-tree. And even with something like this I have a problem of figuring out how to perform this action and keeping XState's core not aware of @xstate/react's special needs.

[davidkpiano]

I am completely fine with this not working for invoked/spawned machines. Only the immediate machine passed into useMachine(machine) should be concerned with actions executed in useEffect/useLayoutEffect, otherwise we're trying to control absolutely everything that can happen anywhere, which is unrealistic.

[Andarist]

If this is not intended to work then we should try to implement a warning if somebody tries to do this.

However, I would be quite surprised that I wouldn't be able to use this for a nested machine in an example like this: https://codesandbox.io/embed/33wr94qv1 (the official TODO example). This limitation is not apparent at all to a user, so we should at least try to surface it is a problem.

[davidkpiano]

Actually, now that I think about it, it can work as intended for nested machines. All that asEffect and asLayoutEffect does is wrap an action in a special function, so if we change useService to detect those actions too, we can schedule those in useEffect and useLayoutEffect all the same.

[davidkpiano]

I'll do this in the follow-up PR to refactor useService and add useActor.

[davidkpiano]

Yes, you're right.

[Andarist]

Then useActor couldn't be used to make this work for nested machines - one thing is that it probably shouldn't know anything about what it receives from being subscribed and thus it shouldn't special case received state.actions. The other thing is that you can have nested machines without actually using useService/useActor on them, and you could also call useService/useActor multiple times on a single service - and we wouldn't like to fire those effects multiple times.

[davidkpiano]

Thinking about this more: this is an invalid use-case. Since useService is a shared single instance of a started machine, and can be potentially used in many components, we cannot "schedule" the same effect to run at different times for each component; that effect should only fire once.

If the service is created as useMachine in some component, it is that component's responsibility to schedule actions via asEffect or asLayoutEffect.

What we should do here is add a helper hook for allowing components to react to actions from useService; in other words, "I observed that the service did this action, so I will do this action as a reaction to that".

[davidkpiano]

Here's what that hook would essentially do:

const inputRef = useRef(null);
const [state, send] = useService(someService); // or useActor, doesn't matter

// hook implementation 
const actionsMap = useMemo(() => ({
  focus: () => { inputRef.current?.focus() }
}, [/* ... */]);

useEffect(() => {
  state.actions.forEach(action => {
    actionsMap[action.type]?(state.context, state.event, {
      state,
      _event: state._event,
      action
    })
  });
}, [state]);

[Andarist]

nit: remember that you can't rely on state like that to fire actions, this has to be scheduled through onTransition or similar

[davidkpiano]

@Andarist ping for final review 🙏

[danielkcz]

FYI @davidkpiano Wanted to build latest master to get these changes to my project and TS is complaining here

No idea why is it happening though. The only hotfix I was able to figure out is to remove the return type here.

https://github.com/davidkpiano/xstate/blob/f96ade68e056ecbcbd6df9d165e2539adb9f6c6e/packages/xstate-react/src/useMachine.ts#L59

[davidkpiano]

Are you using the latest TypeScript? I do not get this error.

[danielkcz]

There is TS 3.8.3 in yarn.lock, so that's not the latest.