-2

You guys have probably seen this error, if not it usually looks something like this:

Can't perform a React state update on an unmounted component.
This is a no-op, but it indicates a memory leak in your application.

Firstly this isn't a question about what the error means. It seems to me that there are some cases when this warning is (as far as I can tell) a bit unjustified.

Consider this component

() => {
  const [foo, setFoo] = useState(false)

  return (
    <>
      <p>Is foo: {foo}</p>
      <button onClick={() => {
        setFoo(true)
        window.setTimeout(() => setFoo(false), 1000)
      }}>Click me</button>
    </>
  )
}

Ok, so in this contrived example we wanna show Is foo: true for 1 second after the button is clicked. If the component unmounts during that second it's going to call setFoo and we'll get that warning. As far as I can tell, this isn't a memory leak. It might be holding onto setFoo a bit longer than necessary but this won't build up over time, the timer will release setFoo albeit slightly later than it could. Am I missing something? My boss is telling me to get rid of the warning but to cancel the timeout adds a bunch of complexity to something which shouldn't be complex, and imo not going to offer any meaningful performance benefit.

This lead me to this idea:

export default () => {
  const [state, setState] = useState(true)
  useEffect(() => () => setState(false), [])
  return state
}
const useSafeState = <T extends unknown>(defaultState: T): [T, (newState: T) => void] => {
  const [state, setState] = useState<T>(defaultState)
  const isMounted = useIsMounted()

  return [
    state,
    (newState: T) => {
      if (isMounted) {
        setState(newState)
      }
    },
  ]
}

by changing the first example to use useSafeState it automatically checks, effectively suppressing the warning. What do you guys think of this? Use safe state functionally does nothing, because as the react warning states it's a no op. I feel like the warning is there for when people forget to cancel their setIntervals from their useEffect, but for these other scenarios we're forced to do something silly like this just to get rid of the warning.

Thoughts?

4
  • you need to clear the setTimeout in the useEffects return Commented Oct 22, 2020 at 0:53
  • @DerekPollard hey, thanks for the response. My question is more about why/if we actually need to clear it in some situations. Especially ones where the timeout is being created in response to an event like onClick as opposed to in an effect. If you think it's necessary create a separate useEffect and keep track of the event handle (which is kind of messy) could you explain why? Commented Oct 22, 2020 at 0:58
  • 1
    Yes, you need to do it, this is an indicator of a memory leak Commented Oct 22, 2020 at 0:59
  • 1
    Its always good to be responsible and clean up resources which are no longer used. In this case you would keep a reference to setFoo around, preventing it from being vacuumed. Here you are using a timeout so, the timeout would eventually run. But it could also cause trouble down the road if someone decided to lift the foo state out of this component and you would affect the state after leaving this component as the timer wasn't being stopped. Commented Oct 22, 2020 at 1:18

2 Answers 2

1

You need to clear out the timer in useEffect's return value:

Working example


function TimeoutExample() {
  const [foo, setFoo] = useState(false)
  const counterRef = useRef(null);

  useEffect(() => {
    return () => {
      if (counterRef.current) {
        clearTimeout(counterRef.current);
        counterRef.current = null
      }
    }
  }, []);

  const handleButtonClick = () => {
    if (counterRef.current) return;
    setFoo(true);
    counterRef.current = setTimeout(() => setFoo(false), 1000);
  };

  return (
    <>
      <p>Is foo: {foo}</p>
      <button onClick={handleButtonClick}>Click me</button>
    </>
  )

}


1
  • 1
    If you are a bad user and click the button multiple times here it would lose track of existing timeouts as it only keeps track of the last one. So if you were to click 10 times (within the 1000 ms) and then unmount the component only the last timer would be cleared, and 9 warnings should follow :) Commented Oct 22, 2020 at 1:12
0

React hooks documentation offers some examples on how and why one might need to do cleanup.

Earlier, we looked at how to express side effects that don’t require any cleanup. However, some effects do. For example, we might want to set up a subscription to some external data source. In that case, it is important to clean up so that we don’t introduce a memory leak! Let’s compare how we can do it with classes and with Hooks.

While the setTimeout example may be contrived, it would be better to handle cleanup as required over using something like the useSafeState as that would not necessarily avoid potential memory leaks, even if it in this case block the call to setState. Might save you from a crash or leave less things running for your user improving their experience on your site!

Using the return value of useEffect you can clear the current timer when the component ends up being unmounted by returning a function to do cleanup.

Your component could look something like this:

() => {
  const [foo, setFoo] = useState(false)
  const timeout = useRef(null);

  useEffect(() => {
    return () => {
      if (timeout.current) {
        clearTimeout(timeout.current);
      }
  }, [])

  const handleClick = () => {
    setFoo(true);
    
    if (timeout.current) {
      clearTimeout(timeout.current);
    }
    
    timeout.current = setTimeout(() => setFoo(false), 1000);
  }

  return (
    <>
      <p>Is foo: {foo}</p>
      <button onClick={handleClick}>Click me</button>
    </>
  )
}
8
  • Having a click based useEffect is considered bad practice; it isn't directly obvious what is initiating the timer which causes DeX issues
    – Justice
    Commented Oct 22, 2020 at 1:52
  • @Justice DeX issues? DX? developer experience? Or something else? I agree its not directly obvious in one sense. Even if you have the dependency array hinting at the source. Please elaborate on the bad practice tho! Commented Oct 22, 2020 at 2:02
  • Yes, developer experience - side effects like these should be kept in a useCallback or arrow function (depending on how particular about rerender management).
    – Justice
    Commented Oct 22, 2020 at 2:09
  • Otherwise, you end up in a situation where you're reliant on an external side effect tangentially related to the click handler.
    – Justice
    Commented Oct 22, 2020 at 2:10
  • 1
    @Justice true, especially if you want to change it outside of this without the same effect. But the example was kind of small scale. But agreed on co-locating side effects. For that it would make more sense to create a handler function like in the other answer. For example here if it should always show for 1 second since the last interaction it wont either. Commented Oct 22, 2020 at 2:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.