React: useState is unsafe

[AlexanderShushunov]

useState return "unsafe" setter. If a component has been unmounted and the setter is called we will get a warning. In a class component we can handle unmount event and avoid this situation.
Example: https://codesandbox.io/s/vmm13qmw67
Click the button and get a warning in the console.
Cleanup logic will be very complicated in this case. Because we should separate unmount and "after render" cleanup.

[heyimalex]

Handle it in useEffect by returning a function that cancels the set call on unmount?

useEffect(() => {
  let unmounted = false;
  apiCall(n).then(newN => {
    if (!unmounted) {
      setN(newN);
    }
  });
  return () => {
    unmounted = true;
  };
});

[AlexanderShushunov]

I have thought about the issue again.
There are two common cases. Api call depends on props or not.
If it does not, @heyimalex's solutions fits very well:

 useEffect(() => {
  let unmounted = false;
  apiCall().then(result => {
    if (!unmounted) {
      setter(result);
    }
  }, []);
  return () => unmounted = true;
}); 

If it depends on props, we should pass them as the second argument

 useEffect(() => {
  let changedAfterOrUnmounted = false;
  apiCall(prop).then(result => {
    if (!changedAfterOrUnmounted) {
      setter(result);
    }
  }, [prop]);
  return () => changedAfterOrUnmounted = true;
}); 

This solution has an additional benefit. If props has been changed, api call with old props values result is not needed.
It helps us to avoid a mistake: the result of the previous api call was received later, than the result of the last call. So we set not consistent value in the state.

So, I think it is not a bug :)

[gaearon]

Yep this works as intended. You could also abort the request in the effect cleanup (which was pretty annoying to do in a class).

In practice Suspense is intended to be the primary data fetching mechanism. useEffect has some rough edges but with time you shouldn’t need to use it too often.

[mariuskava]

can I use async/await pattern in this case somehow?

useEffect(async () => {
    let unmounted = false;
    let newN = await apiCall(n)

    // unmounted is always true here
    if (!unmounted) {
        setN(newN);
    }
    return () => unmounted = true;
}, []);

[gaearon]

Edit: this is probably better: https://github.com/facebook/react/issues/14113#issuecomment-467492761

Not like this, no. You can do this though.

const isMounted = useRef(true);

useEffect(() => {
  performCall();
  return () => {
    isMounted.current = false;
  };
}, []);

async function performCall() {
    let newN = await apiCall(n)
    if (isMounted.current) {
        setN(newN);
    }
}

Longer term, Suspense will be the recommended solution instead.

[ghengeveld]

Can you elaborate why Suspense is the recommended solution over using hooks? API wise I can imagine hooks being more flexible.

[gaearon]

I mean that Suspense is the intended API for data fetching because it would look something like

function YourComponent() {
  const data = YourAPIResource.read();
  return <h1>hello, {data.name}</h1>;
}

No need to handle effects, race conditions, etc, by yourself.

API wise I can imagine hooks being more flexible.

You can use Suspense inside of Hooks if you need for some reason, but for many use cases Suspense alone should be enough.

[ghengeveld]

Ah, I was wondering if Suspense could be used from within a hook. Looking forward to seeing that in action.

Totally missed the useRef hook by the way, it's exactly what's needed to deal with isMounted or other things you'd previously assign to the component instance (i.e. this.mounted = true).

[ghengeveld]

The naming of useRef isn't very intuitive. It's easy to confuse with component ref. What about useInstanceValue?

[gaearon]

It's easy to confuse with component ref

It's the same concept though:

const ref = useRef();
// ref.current is DOM node
return <div ref={ref} />

We're intentionally "widening" its meaning. It's also similar to what refs in OCaml mean.

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

[Steffi3rd]

Not like this, no. You can do this though.

const isMounted = useRef(true);

useEffect(() => {
  performCall();
  return () => {
    isMounted.current = false;
  };
}, []);

async function performCall() {
    let newN = await apiCall(n)
    if (isMounted.current) {
        setN(newN);
    }
}

Longer term, Suspense will be the recommended solution instead.

Finally a easy solution to "cleanup" useEffect()

Thanks a lot @gaearon.

(I'm using Gatsby, Suspense is not support yet) :(

[gaearon]

For what it's worth this is probably better because it can work for multiple fetches (e.g. if route params change):

useEffect(() => {
  let canceled = false;

  async function performCall() {
    let newN = await apiCall(n)
    if (!canceled) {
      setN(newN);
    }
  }

  performCall();
  return () => {
    canceled = true;
  };
}, []);

[alecmerdler]

How does this not violate the Rules of Hooks?

[heyimalex]

@alecmerdler which rule?