We can't rely on useMemo as a semantic guarantee

[hnordt]

No description provided.

[esrch]

I know that this has been merged, but this implementation of useMachine has major issues :

• Including if (state.changed) will not update the React state if the context is updated through an action set on an internal self-transition
• Starting the service directly in the useRef will start a new service on each render, which will reduce performance and might increase the memory usage (I guess?).

I am terrible at using git, so I don't feel comfortable making a pull request, sorry...

[hnordt]

Including if (state.changed) will not update the React state if the context is updated through an action set on an internal self-transition

Could you confirm that @davidkpiano ?

Starting the service directly in the useRef will start a new service on each render, which will reduce performance and might increase the memory usage (I guess?).

I decided to avoid using the "hack" const [service] = useState(() => interpret(...)) on this PR because we didn't measure. I'm not sure if it's really gonna reduce performance and increase memory usage. We need to run benchmarks otherwise we are early optimizing.

[hnordt]

@gaearon your thoughts would be much appreciated here. We are using useRef() to create a XState's service instance.

@esrch left a comment saying that starting the service directly with useRef will recreate the service on each render, which might reduce performance and might increase the memory usage.

What do you say? It's worth optimizing it?

[esrch]

For the first problem, here is a codesandbox showing that using if (state.changed) doesn't rerender on internal self-transitions. Note: I changed the code of useMachine to reference service.current rather that just service, otherwise it didn't work.

For the second problem, I don't know what the performance profile is, but a cleaner approach would seem to be the following:

import { useState, useRef, useEffect, useCallback } from "react"
import { interpret } from "xstate"

export function useMachine(machine) {
  const [current, setCurrent] = useState(machine.initialState)

  const machineRef = useRef(machine)
  const serviceRef = useRef(null)

  const getService = useCallback(
    () => {
      if (machineRef.current !== machine) {
        machineRef.current = machine
        serviceRef.current = interpret(machine).onTransition(state => {
          setCurrent(state)
        })
      } else if (serviceRef.current === null) {
        serviceRef.current = interpret(machine).onTransition(state => {
          setCurrent(state)
        })
      }
      return serviceRef.current
    },
    [machine],
  )

  useEffect(
    () => {
      // Start the service when the component mounts
      getService().start()

      return () => {
        // Stop the service when the component unmounts
        getService().stop()
      }
    },
    [getService],
  )

  return [current, getService().send]
}

This doesn't reinterpret (or restart) the machine on each render, updates the service if the machine changes, and avoids any "missing dependencies" warnings on useEffect.

Here is the machine in action.

[davidkpiano]

The first issue is solved: #397

The second issue is also fixed in master (service.start() happens in useEffect instead of useRef). Also, w/r/t useRef:

The returned object will persist for the full lifetime of the component.

[esrch]

That's great for the first issue!

For the second issue, the useMachine implement in the todomvc example still starts a service on every render.

I believe there are still two "problems" with the current implementation in master:

1. While the interpreter returned by useRef will persist for the full lifetime of the component, the machine will still be reinterpreted on every rerender, since the code interpret(machine).onTransition(...) will be run every time useMachine is called, which is on every rerender. This might or might not have a big impact, but it is unnecessary.
2. The bigger issue for me is that if for some reason the machine passed to useMachine changes, the intuitive behavior should be to stop the running service, and start a new one with the new machine. The current implementation doesn't do that, since the service ref is not updated when the machine changes. Adding machine to the dependencies of useEffect doesn't change that, since service.current sends the same interpreter each time.

I don't want to belabor the point, but I do think it is important that the suggested useMachine implementation in the docs is as robust as possible. Most people will probably copy-paste the code from the docs, and keep it at that. We might as well make it as foolproof as possible to avoid issues and questions down the road.

Here is again my suggested implementation, with additional comments to make it easier to understand. I don't know if it is perfect, and I would be glad for suggestions, but it does address the issues I mention above.

import { useState, useRef, useEffect, useCallback } from "react"
import { interpret } from "xstate"

export function useMachine(machine) {
  const [current, setCurrent] = useState(machine.initialState)

  // Keep a reference to the machine for the running service
  const machineRef = useRef(machine)
  // Keep a reference to the running service
  const serviceRef = useRef(null)

  const getService = useCallback(
    () => {
      // On the first run, set the interpreter/service
      if (serviceRef.current === null) {
        serviceRef.current = interpret(machine).onTransition(state => {
          setCurrent(state)
        })
      }
      // If the machine has changed, update the reference, and update the interpreter/service
      else if (machineRef.current !== machine) {
        machineRef.current = machine
        serviceRef.current = interpret(machine).onTransition(state => {
          setCurrent(state)
        })
      }
      return serviceRef.current
    },
    // Update when the machine changes
    [machine],
  )

  useEffect(
    () => {
      // Start the service when the component mounts
      getService().start()

      return () => {
        // Stop the service when the component unmounts
        getService().stop()
      }
    },
    // Restart the service with the new machine when getService changes
    [getService],
  )

  return [current, getService().send]
}

[davidkpiano]

if for some reason the machine passed to useMachine changes...

This should never happen, and should not be allowed. The machine represents application logic, and should be the most static, unchanging part of the application. Allowing the machine to change at any given time leads to a lot of non-deterministic edge cases and complexity.

As for the implementation, I feel it's more complicated than it needs to be. useCallback seems redundant and can be replaced by just using useEffect:

// ...
const serviceRef = useRef(null);

useEffect(() => {
  if (!serviceRef) {
    // define and initialize service
  }
  // else do nothing

  return () => { serviceRef && serviceRef.stop() }
});

Again, let's assume that the machine will never change. We can enforce this in the hook, as well.

[esrch]

Assuming that the machine can't change certainly makes the code much simpler. I think it would be good to make this assumption clear in the docs.

If running interpret(machine).onTransition(...) on every render has basically no cost, then the current implementation in master is the simplest, and I apologize for my meandering thoughts 🙇

Otherwise, we could go with your idea to define and initialize the service in useEffect, but the serviceRef must be initialized to an empty object. The function argument of useEffect is run after the first render, so when you return serviceRef.current.send from the hook, you get an error if serviceRef is initialized to null ("Cannot read property 'send' of null"). A simple implementation could be:

import { useState, useRef, useEffect } from "react"
import { interpret } from "xstate"

export function useMachine(machine) {
  const [current, setCurrent] = useState(machine.initialState)

  const serviceRef = useRef({})

  useEffect(() => {
    serviceRef.current = interpret(machine)
      .onTransition(state => {
        if (state.changed) {
          setCurrent(state)
        }
      })
      .start()

    return () => {
      serviceRef.current.stop()
    }
  }, [machine])

  return [current, serviceRef.current.send]
}

In this case, the one gotcha that should maybe be mentioned in the docs is that if you want to send an event just after using useMachine, 1. it must be done in a useEffect hook, 2. one must check that send is not undefined, and 3. send must be set as a dependency of useEffect, e.g.,:

function Component() {
  const [state, send] = useMachine(machine)
  useEffect(() => {
    if (send) {
      send('EVENT')
    }
  }, [send])

  return (...)
}

[johnyanarella]

I've been rolling with this:

import { useEffect, useRef, useState } from 'react'
import { interpret } from 'xstate'

export function useStatic(lazyInitializer) {
  const ref = useRef()
  if (ref.current === undefined) {
    ref.current = lazyInitializer()
  }
  return ref.current
}

export function useMachine(machine, options) {
  const [state, setState] = useState(machine.initialState)

  const service = useStatic(() =>
    interpret(machine, options)
      .onTransition(state => {
        if (state.changed) {
          setState(state)
        }
      })
  )

  useEffect(() => {
    service.start()
    return () => service.stop()
  }, [service])

  return [state, service.send]
}

Very similar to the above, but with the benefit of service.send() being available (for immediate reference in handlers, etc.) on the first render, before useEffect() runs.

[johnyanarella]

Perhaps worth noting, that useStatic() hook also comes in handy for me elsewhere on the rare occasion that I need to create a machine to pass to useMachine() within a functional React component (rather than outside it) because props influence the initial config or context - i.e. via Machine()’s second and third parameters or withConfig() and withContext() builders.

I’m not necessarily advocating that XState should export a useStatic() - but, perhaps there should be two hooks - const machine = useMachine(config, options, initialContext) and const [service, send] = useInterpreter(machine, options) that utilize it (or embed that same approach for immediate one-time lazy initialization) internally?

Or it might get confusing and lead people to expect props could affect the config/options/initialContext beyond the first render. I like the clarity of intention the useStatic() hook brings to this useRef() pattern in the cases where I need it. Perhaps more precise names (for the hooks or their parameters) in that two hook scenario could alleviate that confusion.

[esrch]

@johnyanarella I think your solution is great and that this is what we should use. It is much simpler and avoids creating an interpreter on each render. And as all great solutions, it seems obvious in hindsight 😄

I would just suggest inlining it to make it a bit easier to understand and avoid discussions about naming :

import { useState, useRef, useEffect } from "react"
import { interpret } from "xstate"

export function useMachine(machine) {
  const [current, setCurrent] = useState(machine.initialState);

  const serviceRef = useRef();

  if (!serviceRef.current) {
    serviceRef.current = interpret(machine).onTransition(state => {
      if (state.changed) {
        setCurrent(state);
      }
    })
  }

  useEffect(() => {
    serviceRef.current.start();

    return () => {
      serviceRef.current.stop();
    }
  }, [])

  return [current, serviceRef.current.send];
}

I didn't include the options and initialContext parameters in the code above since they seem to be redundant, given that they could be covered by passing in a configured and initialized machine to useMachine, i.e. useMachine(machine.withConfig({...}) or useMachine(machine.withContext({...}). This should also cover your second use of useStatic, or am I misunderstanding something?

I also think it would be good to include a note in the docs to mention that sending an event to the service before the first render will result in an error ("Unable to send event to an uninitialized service"), which can be solved by doing it in a useEffect hook (this has become a bit simpler with send being defined), e.g.,

function Component() {
  const [state, send] = useMachine(machine);
  useEffect(() => {
    send('EVENT')
  }, [send]);

  return (...);
}

[johnyanarella]

Ha, yeah, I hear you. "Easier to understand" is always in the eye of the beholder. Maybe I can sway you to my point of view, though...

useRef() has two potential purposes (hold a reference to an instance of something vs hold a reference to a React element) and three potential patterns of use (assign an immutable value immediately; repeatedly assign a mutable value, typically in useEffect; and, pass to a React element ref prop to have it assign a DOM reference).

So much complexity in software comes from trying to make one thing do two things.
-Ryan Singer

Not only does useStatic() clarify which purpose and pattern is being used, the .current. adds unnecessary noise on every subsequent reference. Is it better to have to read through every line that relates to the ref to derive the intent, or declare it via a function named after the purpose that encapsulates the pattern? I feel like there's still some value in using it even if it isn't exported.

(Bikeshedding some more, I just realized that that useImmutableRef() makes a much clearer name than useStatic(). Renaming it in my codebase now.)

Even so, I get where you are coming from!

Explicitly doing the useRef() dance inlined in the function probably looks more familiar to anyone who has used them in this manner. (Worth noting - the current React docs only include an example of this pattern deep in the FAQ rather than directly in the useRef() section.)

[johnyanarella]

Still need that options parameter, too. That's used to specify interpreter options, such as:

• whether actions should be executed immediately upon transition;
• providing a custom logger;
• whether to enable Redux devTools support; and
• whether to defer events that are sent before it starts.

See: https://github.com/davidkpiano/xstate/blob/master/packages/xstate-react/src/index.ts#L5-L25

That ambiguity about options is part of what makes me wonder if useInterpreter() is more accurate. For some reason it sure is fun to say "use this machine", though!

[johnyanarella]

Good point about just using the builder when passing the machine into useMachine(). I guess it just irked me to know it was going to run that builder on every render to create a machine (potentially configured with different props) that would be ignored after the first render.

The difference between running the builder and allocating an object that is ignored vs creating an inline closure that will be ignored is probably negligible.

I'm more inclined to follow your suggestion now - just calling the builder adds less complexity than the closure and/or hypothetical useMachine()/useInterpreter() two hook scenario I was beginning to entertain. Thanks!

[hnordt]

I don’t think we should make the code more complex just because we think recreating an object would cause performance issues.

I’d say we should ship as is and wait for real benchmarks or issues that can be reproduced.

[hnordt]

For example, assigning to a ref directly in render phase will cause issues. You need to assign inside an effect, but then you introduce a delay and you are also creating two new objects on every rerender; the callback and deps passed to useEffect are also objects.

We don’t know how expensive is to create the interpreter, so we are basically trying to optimize something without real data. Optimizations made without real data lead to slower code more often than not.

[johnyanarella]

And yet, confusingly, given my last comment...

I'm also still on your side that creating, configuring, and starting (and adding a listener to) an interpreter that will be ignored on every render by passing it directly into useRef() should be avoided.

It seems like a potential leak (stop() won't get called on the ignored instances) and crosses a threshold vs the relatively lightweight (non-side-effectful) cloning of using the withConfig() and withContext() builders.

I was happy to see @hnordt address the useMemo() vs useRef() concern with this PR. (I just said "me, too" in gitter, but he stepped up with a PR!) I'll be happier still to throw the custom useMachine() hook out of my codebase if we can get this last wrinkle ironed out.

[johnyanarella]

@hnordt Just saw that the start() got moved out of the useRef() param (vs the earlier commit). That addresses my biggest concern - that an orphaned machine might fire side-effects like actions/services/activities or unintentionally trigger a setCurrent().

[johnyanarella]

@hnordt Turns out, setting a useRef() ref on the render phase is apparently fine for lazy instantiation. They've since clarified that advice in the documentation.

See: https://reactjs.org/docs/hooks-faq.html#how-to-create-expensive-objects-lazily

Given their documented pattern should probably be considered the idiomatic solution, we could throw out the complexity that my extra custom useImmutableRef() hook would introduce.

[johnyanarella]

@hnordt This is the change that confuses me - useRef() doesn't have a second deps param, so this will start on every render.

https://github.com/davidkpiano/xstate/blob/master/docs/sandboxes/todomvc/useMachine.js#L13-L14

Looks like the doc change is out of sync with the final hook you settled on?

(The comment is out of date here, too - https://github.com/davidkpiano/xstate/blob/master/packages/xstate-react/src/index.ts#L34)

[johnyanarella]

Sorry, Heliton - probably feels like I've been nitpicking your hook PR to death! Rest assured, thankful for your effort - hopefully that last link to the out-of-sync doc example reveals the source of confusion. Can't wait to throw out my current hook and use the bundled one!

[davidkpiano]

After reading all the comments here and going over the docs, this is what I'm thinking:

export function useMachine<TContext, TEvent extends EventObject>(
  machine: StateMachine<TContext, any, TEvent>,
  options?: Partial<InterpreterOptions>
): [State<TContext, TEvent>, Interpreter<TContext, any, TEvent>['send']] {
  // Keep track of the current machine state
  const [current, setCurrent] = useState(machine.initialState);

  // Reference the service (created only once!)
  const serviceRef = useRef<Interpreter<TContext, any, TEvent> | null>(null);

  const service =
    serviceRef.current ||
    ((serviceRef.current = interpret(machine, options).onTransition(state => {
      // Update the current machine state when a transition occurs
      if (state.changed) {
        setCurrent(state);
      }
    })),
    serviceRef.current);

  // Stop the service when the component unmounts
  useEffect(() => {
    service.start();

    return () => {
      service.stop();
    };
  }, []);

  return [current, service.send];
}

Unlike the docs, we don't need to create a getService() function every time because we know the logic for creating the service is only used in one place.

[davidkpiano]

Done here: cebbff0

[johnyanarella]

Clever!

The useMachine() in docs/sandbox/todomvc is still accidentally broken, but I imagine the next step is to change that to depend on @xstate/react and import useMachine instead?

[davidkpiano]

Right, but we still have to think through a couple things:

• Challenging the assumption (mine, admittedly) that machine will never change. The machine might not change, but things like the actions/services/guards/etc. will, especially when props change.
• Preventing the machine from being constructed on every render, which occurs when doing useMachine(machine.withConfig(...)). This isn't necessarily an expensive operation, but still worth preventing (related to first point)

One idea is to combine machine options with the service options (which might be a good enhancement anyway):

const [current, send] = useMachine(someStaticMachine, {
  // machine options
  actions: { /* ... */ },
  services: { /* ... */ },
  guards: { /* ... */ },
  // service options
  devTools: false,
  // ...
});

This allows machine options to be memoized:

const actions = useMemo(() => {
  notify: ctx => { onChange(ctx.user); },
  // ...
}, [onChange]);

const [current, send] = useMachine(someMachine, {
  actions
});

The interpreter code would have to internally be changed to allow an API like service.modify(machineOptions) for machine options (actions, services, guards, etc.) to be updated on the fly. This is "okay" (I believe) because the machine logic will never change, so it's still deterministic.

Thoughts?

[johnyanarella]

Still grappling with that one - my thoughts based on the machines I've written so far:

Imagine a machine where a service performs actions that are influenced by props, and have guards that are influenced by props.

1. If the service begins work (under an initial set of prop values),
2. and the service and guards were to be reconfigured (mid-flight for the service),
3. and the service resolves/rejects/calls-back with an event
4. and that event is handled by guards (now reconfigured under a different set of prop values),
5. ...it feels like things just got a lot less determinisitic.

Like you, my instinct so far has been that machine configuration should remain immutable once started.

That said, you can still vary the behavior based on props, but you model it in the machine via attributes of your events. The actions, service and guards inspect the event attributes for any variable behavior (and the service potentially propagates some attributes in events it emits). The variability is explicitly modelled, rather than enacted externally midstream.

As your event travels through the system, it holds the prop-driven configuration captured at the time it was dispatched, and the underlying machine logic doesn't change out from under it. Prop changes affect events initiated in the future, rather than immediately rewiring the machine.

For example, imagine a form machine that needs to execute an async function from an onSubmit prop within a service when in the submitting step, and it can only be interrupted - i.e. transition back to submitting from within submitting in response to a SUBMIT event - when the canInterruptSubmit prop is true.

When your component (or custom hook with a machine inside) sends events, it passes the current prop values in those events. Rather than swapping out the submit service and canInterruptSubmit guard wholesale when those props change, your baked-in submit service can grab and execute the onSubmit function supplied as a property of the SUBMIT event, and your canInterruptSubmit guard can inspect the canInterrupt attribute of the SUBMIT event directly.

I'm definitely open to reconsidering the above - this is just what has been working for me so far.

[davidkpiano]

Just a note (important): there is no such thing as "in-flight" with state machines. State transitions are always instantaneous, and everything is in a single state at any given point of time.

For example, some task doing something async can't be "in-flight" - it's in one of three states:

1. Idle (not doing anything)
2. Pending (this is what you'd consider "in-flight" I suppose)
3. Complete

Maintaining current prop values will lead to unexpected behavior. The (uncommon) use-case I'm thinking of is this:

const SomeComponent = ({ onChange }) => {
  const [current, send] = useMachine(someMachine.withConfig({
    actions: {
      // What if onChange changes?
      notify: ctx => { onChange(ctx) }
    }
  });

  // ...
}

const App = ({ onChange }) => {
  const [state, setState] = useState({ active: false });

  return (
    <SomeComponent onChange={state.active ? e => onChange(e) : undefined} />
  );
}

Of course, this can be refactored to:

<SomeComponent onChange={e => state.active ? onChange(e) : undefined} />

But the "problem" (well, "feature") in React is that you must assume that any prop can change at any time, even props that you know for a fact shouldn't/won't ever change. 😖

[johnyanarella]

Agreed. But services with callbacks feel like they blur that line - they're less "fire and forget" and it's tempting to think of them more as "in flight - make some future choices asynchronously". (And maybe that's a dangerous misuse of them / or just a bad way to describe what's actually happening.)

If you reconfigure an interpreter, what should it do to the running activities and services associated with the current state? I'm not sure. Maybe the key is to consider that reconfiguration an implicit state change (and re-enter the current state)? I'd need to think more about the implications to have a real opinion on that.

haha - right there with you! I love the way React freed us from syncing retained state in the DOM via a simulated immediate mode, but in exchange we got all this complexity where props can change at any time with no context as to why. If only there were a thing beyond props in the api that expressed constants- to describe things that expressly can't change for the life of the component, so you don't have to spend so much 🤬 time defensively coding against them.

[johnyanarella]

Your example is compelling. Begins to help address the frustration of sending events back up to a parent or context ancestor.

[hnordt]

@davidkpiano we shouldn’t assign refs in rendering phase. That’s the reason React docs created a fn, because you would call it inside an effect, e.g. getService().start()

[johnyanarella]

Conceptually, you can think of refs as similar to instance variables in a class. Unless you’re doing lazy initialization, avoid setting refs during rendering — this can lead to surprising behavior. Instead, typically you want to modify refs in event handlers and effects.

(from https://reactjs.org/docs/hooks-faq.html#is-there-something-like-instance-variables)

I hope they meant this. I think the basis for the "don't assign refs in the rendering phase" advice relates to how many surprising different ways a render can occur. A one-time initialization should (hopefully?) be immune from those considerations, since subsequent renders become immaterial.

But who knows - the more I hear about concurrent mode, the more it sounds like components could be re-run in optimistic branches with different responses for a given useRef() or useState() call relative to their branch. I've been surprised a few times now (like Dan revealing that useMemo() is not guaranteed, and that setState() updaters must have no side effects).

Please let me know if you hear anything more concrete than that single sentence in the docs, as I'd hate to get burned again. 😬

[davidkpiano]

Right, I think it's fine. The reason you shouldn't assign refs in the rendering phase is that the assignment might be called multiple times, but this:

const service = 
  // already exists, skips service creation!
  serviceRef.current

  // doesn't exist yet, creates only once!
  || /* .. create service .. */

is safe because it guarantees that initialization is only done once.

[johnyanarella]

Warming to the service.modify(machineOptions) the more I think about it. For the running activities/services concern I mentioned earlier - maybe the developer just assumes responsibility for transitioning to an appropriate state if they modify the options?

[johnyanarella]

Specifically - I like that the "handling prop change edge cases" doesn't bleed so badly into the machine design - my current machines get simpler.

[hnordt]

@johnyanarella @davidkpiano you are right, thank you. ^^