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

useState is unsafe #14113

Closed
AlexanderShushunov opened this issue Nov 5, 2018 · 14 comments

Comments

Projects
None yet
7 participants
@AlexanderShushunov
Copy link

commented Nov 5, 2018

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

This comment has been minimized.

Copy link

commented Nov 6, 2018

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

This comment has been minimized.

Copy link
Author

commented Nov 6, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

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.

@gaearon gaearon closed this Nov 6, 2018

@mariuskava

This comment has been minimized.

Copy link

commented Nov 6, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

Edit: this is probably better: #14113 (comment)

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

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

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

@gaearon

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

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

@gaearon

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

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

This comment has been minimized.

Copy link

commented Feb 26, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

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

This comment has been minimized.

Copy link

commented Apr 4, 2019

How does this not violate the Rules of Hooks?

@heyimalex

This comment has been minimized.

Copy link

commented Apr 4, 2019

@alecmerdler which rule?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.