statelyai / xstate

Actor-based state management & orchestration for complex app logic.
https://stately.ai/docs
MIT License
27.08k stars 1.25k forks source link

TypeError: Cannot read property 'state' of undefined if interpreter passed to useService is undefined #1416

Closed ivorpad closed 1 year ago

ivorpad commented 4 years ago

Description If an interpreter at any point of the message passing dance between parent and child machines is undefined then the application crashes. I believe this is a bug because you can't use conditional with Hooks.

Expected Result

const [state] = useService(undefined)
// state should be undefined

This is a problem specially when you're passing down the service to a child component:

import React from "react";
import { useService } from "@xstate/react";

function Child({ service }) {
  const [state] = useService(service);
  return <div>{state.event.type}</div>;
}

export default Child;

Once the child machine reaches its final state the service will be undefined, thus the app crashes.

Actual Result

Screen Shot 2020-08-27 at 1 45 01 AM

Reproduction https://codesandbox.io/s/sweet-heisenberg-o9jqx?file=/src/App.js https://codesandbox.io/s/heuristic-dew-b2h23?file=/src/child.machine.js:477-491 (comment L25 → uncomment L26 → click the button)

davidkpiano commented 4 years ago

Not sure if this makes sense. You get the same type of error if you pass undefined as a reducer to the built-in useReducer(undefined, ...) hook.

In that case, you'd want to prevent rendering that component until it has what's necessary for it to run. This is more of a React design concern than an XState problem.

ivorpad commented 4 years ago

@davidkpiano That's not exactly true:

Screen Shot 2020-08-27 at 8 05 27 AM

That's how useService should behave IMHO.

I could prevent rendering that component but what if, according to my app design, that's not what it's intended?

mattpocock commented 4 years ago

This feels like a 'I should be able to conditionally call hooks' discussion. If there are UI elements you want to render, you can pass down the relevant information via props instead of through a service. I think it's reasonable to expect, when you use a hook, it has all the information it needs.

If you need this behaviour, you can write a custom wrapper for useService which checks to see if the service is defined?

Andarist commented 4 years ago

While this is not written by the React team itself in TS useReducer doesn't accept undefined: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/9c7b9007799572d3108ffa5f36887690ceed6b68/types/react/index.d.ts#L937 . No overload allows this.

Even if it works at runtime - it's not said to be a good idea.

Andarist commented 4 years ago

Just an additional comment - it is natural for us to clear children from the stopped child services as otherwise, we'd end up with a memory leak. If you need to preserve the stopped child's state for rendering purposes then you should sync this state with a parent and render based on parent's state. This is close to the classic "lift state up" advice.

davidkpiano commented 4 years ago

@davidkpiano That's not exactly true:

Screen Shot 2020-08-27 at 8 05 27 AM

That's how useService should behave IMHO.

I could prevent rendering that component but what if, according to my app design, that's not what it's intended?

It is definitely true: https://codesandbox.io/s/fragrant-tree-dnbuc?file=/src/App.js

This will throw an error:

export default function App() {
  const [state, dispatch] = useReducer(undefined, 1);

  useEffect(() => {
    dispatch("anything");
  }, []);

  return (
    // ...
  );
}
Andarist commented 4 years ago

@ivorpad's case is slightly different though - he doesn't expect to be able to send events to the received service (after it gets stopped etc), he just wants to read its state and in case of useReducer you can do that (although only at runtime, types disallow this).

davidkpiano commented 1 year ago

This is now invalid in the latest @xstate/react@beta - useService(...) is deprecated, and useSelector(actor, selector) and actor?.send() should be used instead.

Currently, a defined actor must be passed into the first arg of useSelector (or maybeActor && createEmptyActor(), although this restriction may be loosened: #3159