Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

useService and side-effects #547

Closed
hnordt opened this issue Jul 15, 2019 · 33 comments
Closed

useService and side-effects #547

hnordt opened this issue Jul 15, 2019 · 33 comments

Comments

@hnordt
Copy link
Contributor

hnordt commented Jul 15, 2019

In the TodoMVC example, Todo.jsx file we have the following code:

useEffect(() => {
  todoRef.execute(state, {
    focusInput() {
      inputRef.current && inputRef.current.select();
    }
  });
}, [state, todoRef]);

I didn't like that approach because all the state side-effects are going to be executed twice. Once by the interpreter, and again by the useEffect. You can add some console.logs to todoMachine to confirm that. For example, all sendParent() actions are gonna be executed twice.

It's not a big issue, but it's more a workaround than a proper solution.

I don't know how to fix that, but I would like to open that GitHub issue to start some discussion.

@hnordt hnordt changed the title useService and side effects useService and side-effects Jul 15, 2019
@hnordt
Copy link
Contributor Author

hnordt commented Jul 15, 2019

An idea I just had:

const inputRef = useRef(null);
const [state, send] = useService(todoRef.overrideActions({
   focusInput() {
      inputRef.current && inputRef.current.select();
   }
}))

Actor refs could support a new method overrideActions which overrides actions in runtime.

Or maybe we can pass an option to execute() so it executes only the passed actions (Object.keys):

useEffect(() => {
  todoRef.execute(state, {
    focusInput() {
      inputRef.current && inputRef.current.select();
    }
  },
  {
    // execute only `focusInput()` and ignore everything else
    shallow: true
  }
);
}, [state, todoRef]);

@davidkpiano
Copy link
Member

I agree that there should be a different way to execute side-effects from live services, but conceptually overriding actions is impossible. A service can be something that lives anywhere (such as executing on some remote server) where you can't always "stop" actions that are going to execute.

Instead, we can have a "partial execution" function that only executes actions where implementation is provided:

// TENTATIVE API
import { execute } from 'xstate';

// ...

useEffect(() => {
  // This will only execute the 'focusInput' action, and no other action
  execute(state, {
    focusInput() { /* ... */ }
  });
}, [state]);

Thoughts?

@hnordt
Copy link
Contributor Author

hnordt commented Jul 16, 2019

@davidkpiano that was my second suggestion. Maybe we can name it executeOnly.

@Andarist
Copy link
Member

I dont quite get the problem - what does it solve? Logging? Like - there is a need for smth extra over just useEffect?

@johnyanarella
Copy link

johnyanarella commented Jul 16, 2019

Not convinced that useEffect() is the right scheduling mechanism. The rules and timing around a useEffect() handler's execution do not align well with state machine transitions and when other actions for a transition occur. Worse, given React's batching and suspense, it may not fire when expected (or at all).

A better route might be via event dispatching; where the service would emit an "action" event and the component could subscribe to be notified when an external action should be fired. As you would to integrate with other event-dispatching APIs (IntersectionObserver, ResizeObserver, etc.), useEffect() would just be used to subscribe / unsubscribe a handler to that event whenever you needed to close around updated relevant component state (props, refs, etc.).

Supposing that a service.onAction() API like that existed, you could even wrap that up into a custom hook to avoid some boilerplate:

export function useAction(service, action, actionFn) {
  useEffect(() => {
    const listener = service.onAction(action, actionFn)
    return () => listener.unsubscribe()
  }, [service, action, actionFn])
}
useAction(service, 'focusInput', () => inputRef.current.focus())

or

const focusInput = useCallback(() => {
  inputRef.current.focus()
}, [inputRef.current])

useAction(service, 'focusInput', focusInput)

The event dispatching approach would also provide value beyond React + hooks, as it could be used similarly for "late-binding" external actions to a service in components for other frameworks.

@Andarist
Copy link
Member

This approach is definitely better in my eyes - it makes integration with external scheduling system more apparent. Not sure though if "late-binding" is compliant with SCXML - executable content is always known at definition time. This doesn't mean per se that we can't inject implementation lately into such content (action implementation) but the action itself should be registered eagerly (at definition time). This might couple the service to a "child component" behavior, but maybe that's not necessarily bad - and there is always a possibility to split machines/actors so they stay more decoupled.

Maybe there is a way to extend basic SCXML semantics with rules described in Extensibility of Executable Content, but would have to read through it more carefully to see if there is a way.

@hnordt
Copy link
Contributor Author

hnordt commented Jul 16, 2019 via email

@hnordt
Copy link
Contributor Author

hnordt commented Jul 16, 2019

Sorry, on my phone, I’ll spam you guys a bit. 😅

Actually I think useEffect is the right choice for the kind of side effects we want to dispatch: rendering ones.

It’s the UI framework that has more info to know some effect should run, be paused or cancelled. We don’t want to force XState to dispatch rendering effects that doesn’t matter for the UI anymore.

@johnyanarella
Copy link

johnyanarella commented Jul 16, 2019

@Andarist

Perhaps one way to think about it is that a named action that has no specified implementation would have an implicit implementation of firing an event.

Or perhaps it could be better expressed as an explicit API?

// NOTE: conjured "dispatch" out of thin air here - same API as send()
import { dispatch } from 'xstate'

const machine = Machine({
  // ...
},
{
  actions: {
    focusInput: dispatch('focusInput')
  }
})

(Where the interpreter would accept subscriptions for and dispatch the specified event.)

Unlike actually reconfiguring the machine at runtime, one could argue this gives us some useful effects of late-binding (i.e. independent entities such as a React component are sent a message that they handle as they deem fit relative to their internal state) without it really being late binding (i.e. the behavior of the machine itself changing in any way). The implementation for the action is known at definition time - the named action will dispatch an event.

@johnyanarella
Copy link

johnyanarella commented Jul 16, 2019

@hnordt

It makes me uneasy that the limitations will be non-obvious and the behavior unpredictable.

A useEffect() handler is going to fire at some point in the future with the most recent state relative to whenever React chose to commit a batch. So, any action that was intended to be triggered by intermediate transition(s) prior to that won't be honored. Behavior ends up being entirely indeterminate, only working when the slices happen to line up just right.

It’s the UI framework that has more info to know some effect should run, be paused or cancelled. We don’t want to force XState to dispatch rendering effects that doesn’t matter for the UI anymore.

I definitely agree that useEffect() and the component implementation should make the decisions about whether a given imperative UI effect it manages should run (scroll to here, focus that) or be interrupted or cancelled. It's the only one that knows enough to make those decisions...

...but it also needs to know what actions were requested (aka messages were sent) in between the slices of time when it runs so that it has the rest of the information necessary to make those decisions.

And the sanest way is to set up a channel of communication where the XState machine can tell it (because it's the only one that knows and a single current state snapshot won't be enough). Since useEffect() runs at React's whim based on a different cadence, I don't think it's the right channel. It's like taking snapshots at random intervals and trying to derive what happened, rather than getting explicit play-by-play updates.

Given your point, I think the body of my useAction() handlers above are wrong. The useAction() handlers should be setting some state (maybe via a useState() setter) in the component that it later commits in a useEffect().

@johnyanarella
Copy link

@hnordt

If you have the time, we could simulate and play around with the idea I was proposing above without any XState changes to see if there are showstoppers I'm not seeing.

import mitt from 'mitt'

const machine = Machine({
  context: {
    emitter: mitt()
  },
  states: {
    inactive: {
        on: {
          TRIGGER: {
            target: 'active',
            actions: ['focusInput']
          }
        }
      }
    },
    // ...
  }
},
{
  actions: {
    focusInput(context, event) {
      context.emitter.emit('focusInput', /* ... */)
    }
  }
})
export function useEmitter(emitter, type, handler) {
  useEffect(() => {
    emitter.on(type, handler)
    return () => emitter.off(type, handler)
  }, [emitter, type, handler])
}
useEmitter(context.emitter, 'focusInput', (event) => {
  // whatever we should actually be doing here to schedule a potential focus()
})

@Andarist
Copy link
Member

Unlike actually reconfiguring the machine at runtime, one could argue this gives us some useful effects of late-binding (i.e. independent entities such as a React component are sent a message that they handle as they deem fit relative to their internal state) without it really being late binding (i.e. the behavior of the machine itself changing in any way). The implementation for the action is known at definition time - the named action will dispatch an event.

Yep, I had smth like this in mind with "This doesn't mean per se that we can't inject implementation lately into such content (action implementation) but the action itself should be registered eagerly (at definition time).".

Given your point, I think the body of my useAction() handlers above are wrong. The useAction() handlers should be setting some state (maybe via a useState() setter) in the component that it later commits in a useEffect().

Yeah, this would probably need to be based on 2 effect callbacks cooperating, smth like:

export function useAction(service, action, actionFn) {
  const actionsRef = React.useRef([])
  
  useEffect(() => {
    const listener = service.onAction(action, (v) => {
      actionsRef.push(v)
    })
    return () => listener.unsubscribe()
  }, [service, action])

  // no deps here, as we assume that we fire post-render actions?
  React.useEffect(() => {
    const currentActions = actionsRef.current
    actionsRef.current = []
    currentActions.forEach(v => actionFn(v))
  })
}

Because of the arbitrary time in which the second effect's callback might get called the cancellation mechanism should be supported as well.

@hnordt
Copy link
Contributor Author

hnordt commented Jul 16, 2019

@johnyanarella

It makes me uneasy that the limitations will be non-obvious and the behavior unpredictable.

That's also a good point. 👍 I agree we should improve the behavior and make it more predictable.

Unfortunately I'm very busy this week, but I'll try to find some time to play with your proposed API.

@johnyanarella
Copy link

@hnordt

All evidence to the contrary (so many words above!), I'm right there with you - busy week ahead. Best of luck with yours!

@vanilla-wave
Copy link

I faced a similar problem. I needув to handle side effect in component when machine dispatch action.
I used service.subscribe and checked dispatching actions. I created hook.

const useServiceSubscribe = (service: ServiceType, actionMap: SubscribeMap) => {
    useEffect(() => {
        const subscription = service.subscribe((state) => {
            state.actions.forEach(action => {
                if (actionMap[action.type]) {
                    actionMap[action.type](state.context);
                }
            });
        });

        return subscription.unsubscribe;
    }, []);
};

In component i can use it

useServiceSubscribe(service, {
        'action1': (ctx) => {
            // do smth
        },
        'action2': (ctx) => {
            // do smth
        },
    });

Does this solve the described problem, or am I missing something?

@CodingDive
Copy link
Contributor

CodingDive commented Feb 16, 2020

I'm running into the same problem with services when invoking a promise. The promise is being executed twice.

The parent machine is responsible for invoking a promise and to change states depending on the response, while the child machine is responsible for sending the event to the parent.

// inside the parent component, I inject some actions and ref.execute causes the service to be executed twice
useEffect(() => {
  parentRef.execute(state, {
    /** Some actions that are unrelated to the service that is being executed twice*/
  });
}, [state, parentRef]);

(grandParentMachine invokes parentMachine which invokes childMachine)

Any idea how I could prevent the service from firing twice? It's a HTTP request that upon sending a second time, the server will always reject, causing the onError handler to be called.

@vizet would your solution also work with services or only actions?

@Andarist
Copy link
Member

@CodingDive could u prepare a runnable repro case?

@davidkpiano
Copy link
Member

Also, you can specify effects as the second argument:

const [state, send] = useMachine(someMachine, {
  actions: {
    // your custom actions
  },
  // other custom options
});

Anything passed to actions, services, etc. will be updated internally, so the machine will never reference stale values when executing side-effects.

@CodingDive
Copy link
Contributor

CodingDive commented Feb 16, 2020

Here is the repro. https://codesandbox.io/s/magical-cloud-pv0i1
Click on the button and check the console. Commenting out the useEffect of line 8-12 in the Parent.tsx file will only execute the service once.

@CodingDive
Copy link
Contributor

@davidkpiano what do you mean with updated internally? Services might still be executed twice but they won't have stale values or something different?

@Andarist
Copy link
Member

This happens because you are using execute with the latest state which holds to last executed actions and thus you are re-executing them:
https://github.com/davidkpiano/xstate/blob/a6d7d28a42f7b57083c5cc85db2132773c07be71/packages/core/src/interpreter.ts#L246-L252

I wouldn't recommend this pattern of calling execute from the effect based on the state. You should rely on how XState executes actions on its own.

@davidkpiano what do you mean with updated internally?

It has just meant that you can do this:

useMachine(someMachine, {
  actions: {
    someAction: () => /* reference react state or whatever, avoiding stale closure problem */
  }
}

@CodingDive
Copy link
Contributor

Thank you for the clarification. I'd normally pass actions or services to the useMachine but when I'm already working with a service/actor reference (parentRef), I'm not aware how to inject actions other than calling ref.execute within the useEffect.

How could I pass some actions to the interpreter without reexecuting the service? Most actions I pass in there are not reliant on the machine state at all. E.g when focusing an input field using useRef. I believe this is also the example of the todoMvc but it obviously doesn't deal with 3 machines in a hierarchy and also no data fetching.
Hoisting all of the actions that I need to pass from react land to the very top of my actor hierarchy, where I can actually call useMachine, is an option but it would complicate the communication quite a bit.

@davidkpiano
Copy link
Member

but when I'm already working with a service/actor reference

When you're already working with a live service instance, you can't stop it from executing side-effects. It's a shared instance with side-effects that will execute, so you deciding to also useEffect(...) to execute your own side-effects on top of that must take that into consideration.

If you want to share a service that does not execute side-effects (and instead hand that responsibility to child components), then you must model it as such.

@Andarist
Copy link
Member

Hoisting all of the actions that I need to pass from react land to the very top of my actor hierarchy, where I can actually call useMachine, is an option but it would complicate the communication quite a bit.

Well - if you made particular responsibility a part of a particular machine then I would say that it's much better to colocate this stuff with it. It's basically a similar problem (& solution) to React's lifting state up

@CodingDive
Copy link
Contributor

CodingDive commented Feb 17, 2020

It's a shared instance with side-effects that will execute, so you deciding to also useEffect(...) to execute your own side-effects on top of that must take that into consideration.

It's not that I necessarily want to use useEffect, I just want a way to pass an action to the live service.

I guess as of now, I could either pass the action to the event of the grandParent machine which spawns the service. That feels rather dirty:
parentRef: (context, event) => spawn(parentMachine.withConfig({ actions: event.actionDump})

Or move every action to the top of the hierarchy chain. What I don't like about this is that the topmost actor needs to define events and actions that are actually the responsibility of the child components and machines. Like the todosMachine defining an action & event to focus the input when it'd be much better suited inside the todoMachine or even todoInputMachine (if it existed).

Now I also understand what this discussion is about as I've used many useEffect hooks to inject actions before and was never bothered by the actions firing twice until running into the promise service issue. There are some actions you definitely don't want to have execute twice.

For my ideal API to solve this, I'd love the (live) Interpreter to move closer to the StateMachine API. Something like the following.

// inside the TodoInput component which is responsible for managing the hypothetical todoInputMachine.
const inputRef = useRef<HtmlInputElement>(null)
const [state, send] = useService(
  todoInputRef.withConfig({
    actions: {
      focusInput: (context, event, state) => void inputRef?.current?.focus(),
    },
  }),
);

return <input ref={inputRef} value={state.context.newTodo} onChange={(e) => void send({ type: ' NEW_TODO', input: e.target.value }) />

I think this would fix the stale state issue as the third param of the passed action is the stateDump of the service by the time the action is called.
I wouldn't want to allow withContext as the component have no business in replacing the extended actor state. Maybe a different name than withConfig wouldn't be bad either.

What do you think?

My proposed API is quite similar to @hnordt overrideActions even though it would just merge the config like we are used to from the machine at the top of the actor hierarchy. To his proposal @davidkpiano responded:

A service can be something that lives anywhere (such as executing on some remote server) where you can't always "stop" actions that are going to execute.

Wouldn't this be the responsibility of the machine or of the component that passes the actions to ensure the actions are stopped/never executed? I believe all my actions have been synchronous so far so I'm not aware how an action can be stopped once it's started. One thing I do is to send events that would trigger actions and if I need to, cancel them before they reach the machine (e.g with delayed events) so that the action is never executed in the first place.

@Andarist
Copy link
Member

Like the todosMachine defining an action & event to focus the input when it'd be much better suited inside the todoMachine or even todoInputMachine (if it existed).

This kinda baffles me - maybe I don't understand the whole context that you have in mind.

On the one hand - you want to have this managed inside todosMachine, but on the other hand you want to only have it partially. Which exact parts would you like to have at which place? Seems like you want to have events and actions higher in the tree (parent/grandparent component) but you want to have an action's implementation closer to a consuming component. And this is mostly dictated by the fact that only there you have access to the underlying ref which you need to focus. Is that right?

Imagine implementation without considering React and how you structure its components etc. Would you do the same then? Would you try to decouple this implementation from the place where you have all the other pieces defined? I think that I wouldn't - this only introduces some indirection which in this instance, I feel, should be avoided.

You are having those machines coupled to each other anyway because their concerns are highly connected - the one higher in the tree serves as a manager/supervisor to the inner ones (among other things). From what I understand - it has the knowledge about when this particular action should happen and it "just" needs it to execute.

It seems like what you have mentioned - creating todoInputMachine - is the real answer here. If you implement it then you will be able to just send a directed command to it (FOCUS_INPUT) and have it react (😉) to it.

The main challenge though is still remaining with such - there is currently no clear answer how to have this todoInputMachine defined together with <TodoInput/> component and yet make the supervising machine aware of it (so it can send an event back to it).

There is a hidden gem that can solve this for you super nicely - you can actually pass parent as an option to the Interpreter and because useMachine starts an interpreter under the hood it forwards all of the received options to it. Check this demo out: https://codesandbox.io/s/wonderful-clarke-8q1y4

That being said - I'm not 100% convinced (I have just literally figured out this solution right now) if we should overload parent like this, or to rephrase - if we should use sendParent for this. It's super convenient though. The alternative that is also supported is that you can just grab parent's sessionId and send to it with send(event, { to: ctx => ctx.parentRef }). The main different between those 2 approaches is that the latter one doesn't establish parent-child relationship between those machines, it just allow for 2 separate machines to communicate with each other.

@davidkpiano - WDYT? I think it would be great to include one of those in the docs. This unlocks some really nice patterns, the only question is - which one should be preferred? I'll give this more thought today as well.

@CodingDive
Copy link
Contributor

CodingDive commented Feb 17, 2020

On the one hand - you want to have this managed inside todosMachine, but on the other hand you want to only have it partially.

I was saying managing this within the todosMachine is the wrong place as the ref and correspondingly the action exists further down the component/machine hierarchy. In other words, if a todoInputMachine exists, I want to pass the action from within a TodoInput component since it's the place where I'd define the input (React) ref. I'd then define an event on the todoInputMachine that executes the action, allowing the parent machine e.g the todosMachine to send the event to its child. I think we were on the same page there all along.

Your solution of passing the parent service into the child component and onto parent is super cool! I love it because it colocates the action with the component where the ref is defined. This allows one to mimic the machine hierarchy with components without hoisting the actions to the machine (and component) at the very top. This is exactly what I wanted. 😍

Nonetheless, I'm pretty convinced that this needs to be solved on the actor level (without useMachine). Otherwise, people are encouraged to speak directly to the parent e.g parentService.send() or even read the parent state which should never be the concern of the ChildComponent that encapsulates the child actor.

Another problem I see is that the parent no longer spawns the child machine itself and instead assigns an instance of the child upon receiving an event.

// inside the parent machine
GREET: {
  actions: assign((ctx, event, { _event }) => {
    return {
      childMachineRef: _event.origin
    };
  })
}

This assumes that the child component which invokes the childMachine (which in turn sends the GREET event) is rendered before the parentMachine gets to work with the childMachineRef. If we want the parent machine to send some events to the child before rendering the ChildComponent, it's even worse as demonstrated in this sandbox. Essentially, since we always need to invoke the ChildComponent for the childMachineRef to be defined inside the parentMachine, we'd need to conditionally assert the parentService state and return null . 🤢

I'd like an API where we can colocate the action of a component by passing it to the actor it is encapsulating (just like demonstrated by your example) while still allowing the parent to be in full control when to spawn the childMachine. (Ideally without breaking the parent-child communication. 😁)

@davidkpiano
Copy link
Member

I only skimmed the above comments (sorry!) but in general, I think there is a missed point about a machine vs. a service:

A machine is a template for creating a service. When you call useMachine(...), the service is created with the actions that you specify, because the server doesn't exist before calling useMachine(...).

When calling useService(...), the service already exists. It's something that's already live, something that's already going to execute the actions it was configured to execute, something that other things can react to and maybe send events to, but not change the behavior of the service.


Here's a loose analogy. If you were the director of a film, and the film hasn't been created yet, you can modify what should happen at certain points of the film, when certain effects should happen, etc. etc.

But if the film was already shot and distributed and playing in theaters, no amount of yelling at the movie theater screen will change what happens in the film.

You can definitely react to the film (and "execute actions" of your own), but you cannot change what happens.

Hopefully that makes more sense.

@CodingDive
Copy link
Contributor

CodingDive commented Feb 19, 2020

Thank you for the great analogy @davidkpiano. With the analogy in mind, whenever I wrote that I want to inject or pass an action into the live service, I should've said: "I want a way to react/subscribe to an action from within React."

I want to use the example of this Codesandbox to make a point why this is fairly important especially since v1 of @xstate/react is just around the corner. As of now, there is no good API to react to an action without useEffect and execute (other than the rather hacky example demonstrated by @Andarist).
Given a GrandParent -> Parent -> Child structure, we want to react to an action in the Parent component. The action which is executed by the parentMachine originates with an event send by the Child component to the childMachine.

// inside the parent component where we subscribe to the parentRef using useService
const [errorMessage, setErrorMessage] = useState(undefined);
// we want React to react to the "SHOW_ERROR" action by modifying some local state.
SHOW_ERROR: (ctx, e) => {
  setErrorMessage(e.data);
}

This is obviously a very contrived example. Instead of setting local state, we could be performing a focus action or anything else.

I can only see three alternatives with a built-in way to manage this. None of which are pretty.

1 Hoist the React code to the GrandParent component, pass it to the config of the grandParentMachine when calling useMachine and have the parentMachine send an event to execute the action.

Tradeoffs:

  • Extra event and action in grandParentMachine that might not be the business of the GrandParent component at all
  • Extra communication between parent- and grandParentMachine
  • Potentially extra props needed between GrandParent and Parent component. E.g passing down a ref from the grandParent component that will only be attached to a DOM element further down, or a function that sets some state for the action closure.

2 Pass props down, send events up. The Parent component could pass the setErrorMessage to the Child component which passes it along to the event.

Tradeoffs:

  • Extra props to pass to child component
  • Extra function to pass to the child event (from child comp to child machine)
  • Extra function to pass to the parent event (from child machine to parent machine)
  • Super awkward to keep the action inside the event especially when using a service (see example below)
src: (context, event) => someAsyncFunction().then((result) => result).catch((error) => Promise.reject({error, setErrorMessage: event.setErrorMessage}),
// just so I can use setErrorMessage as an action in the onError handler
onError: {
  actions: (context, event) => event.data.setErrorMessage(event.data.error)
}

3 Pass the action to the parentMachine context and execute it from there.

I don't think this needs further discussion. This does not seem like a sane idea 😁

onError: {
  actions: (context, event) => context.setErrorMessage(event.data)
}

With the current alternatives out of the way, I also believe that useEffect is the wrong API to react to actions as .execute will call all of the side-effects twice as pointed out by @Andarist.
Davids analogy made a lot of sense to me. I think the API should express that we merely subscribe and react to actions, not pass them into the running service. With the execute API, I originally even thought that the first argument (state) could theoretically be used to directly mutate the state of the service.

I think from a DX perspective, I'd enjoy an API like the following the most.

parentRef.subscribeActions({
  SHOW_ERROR: (ctx, e) => {
    setErrorMessage(e.data);
  }
})

Alternatively, if it shouldn't be handled within the core. When using @xstate/react:

useService(parentRef, {
  subscribeToActions: {
    SHOW_ERROR: (ctx, e) => {
      setErrorMessage(e.data);
    }
  }
})

What do you think?

@davidkpiano
Copy link
Member

Simpler idea:

const ChildComponent = ({ parentService }) => {
  const ref = React.useRef();
  const [state] = useService(parentService);

  // "React" to the "focusChild" action whenever it happens
  useAction(state, "focusChild", () => {
    // should probably be a memoized callback
    ref.current.focus();
  });

  return <input ref={ref} />;
};

https://codesandbox.io/s/distracted-keller-gbhk5

@Andarist
Copy link
Member

Keep in mind that you should not use XState's state as a dependency of an effect, because you might miss stuff when React decides to batch subsequent rerenders.

A more safe version of this would look something like this:
https://codesandbox.io/s/admiring-thompson-o5wbo

I still have to read carefully through @CodingDive's latest post so I can post a comment on all the mentioned concerns etc, but I would like to mention one thing now - I believe that the pattern presented by me is quite powerful and not that much of a hack (maybe besides "connecting" those 2 services in a parent-child relationship). It allows you to split the implementation, decoupling those 2 to some extent in a way that it, for example, allows you to codesplit them, whereas if you embed everything within a single machine definition (referencing other machines directly) you make codesplitting this harder.

@CodingDive
Copy link
Contributor

CodingDive commented Feb 23, 2020

useAction seems indeed like a beautiful API. Since it just works on any service, one can react to child services, which should allow me to spawn actors like I'm used to and prevents me from having to pass down the parentService.

const ChildComponent = ({ childRef }) => {
  const [state, send, childService] = useService(childRef);
  useAction(service, "focusChild", () => {
    ref.current.focus();
  });
}

Since it doesn't use execute, it also won't run any side-effect twice. 🎉 This is exactly what I hoped for. I love it! 😍

@Andarist you can ignore my long posts from above then. With an API like useAction (either putting it in /utils. or if XState decides to ship it), I'm more than happy and all my concerns should be addressed. I'll just reiterate one point of concern about the event-based actor connection pattern below so that you don't have to read through the whole post(s) again.

I believe that the pattern presented by me is quite powerful and not that much of a hack (maybe besides "connecting" those 2 services in a parent-child relationship). It allows you to split the implementation, decoupling those 2 to some extent in a way that it, for example, allows you to codesplit them, whereas if you embed everything within a single machine definition (referencing other machines directly) you make codesplitting this harder.

I believe it can be very powerful too. Pretty sweet that you found this to be possible!

One thing I was worried about is the fact that the child component needs to render and invoke the child machine. Only then (upon having the actor greet the parent service) the child ref becomes available inside the parent service. This makes it difficult for the parent machine to work with the child service before rendering the actual Child component. When I thought about this problem, the only solution I could think of is for the Child component to always be rendered and then have it assert the state of the passed parentService to conditionally render null.

See the codesandbox or the code below

// inside the ChildComponent we always need to call this for the child service to be wired to the parent service
useMachine(childMachine, {    parent: parentService, /*...*/ })

// yikes, we're in the child component and assert the state of the parent service
if (
  !parentService.state ||
  !parentService.state.matches({
    hasChildMachine: "wantToRenderChildComponent"
  })
) {
  return null;
}

// render actual child component elements

Meanwhile, when using the spawn API, the Parent component and machine are always in full control of

1, when to spawn the child machine
2. when to render the child component.

const [state, send] = useMachine(parentMachine); 

if (
  state.matches({
    hasChildMachine: "wantToRenderChildComponent"
  })
) {
  return <ChildComponent childRef={state.context.childRef} /> ;
}

@hnordt
Copy link
Contributor Author

hnordt commented May 25, 2020

Closing. That's the final version based on the previous discussion:

function useAction(service, actionType, callback) {
  const latestCallbackRef = React.useRef(callback)

  React.useEffect(() => {
    latestCallbackRef.current = callback
  })

  React.useEffect(
    () =>
      service.subscribe((state) => {
        if (!state.changed) {
          return
        }

        if (!state.actions.find((action) => action.type === actionType)) {
          return
        }

        latestCallbackRef.current()
      }).unsubscribe,
    [service, actionType]
  )
}

@hnordt hnordt closed this as completed May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants