November 3, 2019

useCustomHookWithRefs

Avoiding A Common Pitfall


Question: What’s wrong with this code?

import {useIsMounted} from './useIsMounted';

function Button({callback}) {
  const isMounted = useIsMounted();
  const onClick = () => {
    if (isMounted) {
      callback();
    }
  }
  return <button onClick={onClick}>Hallo</button>;
}

Answer: if (isMounted) will always return true because isMounted is a ref, not a boolean, and so callback will always be called when the button is clicked, even when it shouldn’t be. There’s no way of knowing that that’s wrong without looking at the useIsMounted definition to see what it returns. The correct form is:

import {useIsMounted} from './useIsMounted';

function Button({callback}) {
  const isMounted = useIsMounted();
  const onClick = () => {
    if (isMounted.current) {
      callback();
    }
  }
  return <button onClick={onClick}>Hallo</button>;
}

This is a common pitfall that combines naming difficulties with JS typecasting difficulties. Even TypeScript wouldn’t catch this bug, since it’s ok to call if(foo) on any type for foo. It’s frustrating, because custom hooks that use useState work pretty much exactly how you’d expect, but custom hooks that return refs can cause confusion if a dev reading over the code doesn’t know what type of value the custom hook is returning. I believe this is what we call a “footgun”.

Right now, my current recommendation for fixing this is to name the hook useIsMountedRef to make it abundantly clear that that’s happening, and encourage naming its return value isMountedRef instead of isMounted. This mostly solves the problem and is ok, but I don’t love it as a solution: it’s not very elegant to have to name a function with the type it returns. I’m curious if anyone else has found a better way. Let me know if you do!

Update: Other Suggestions!

My friend recommended creating a useCallbackIfMounted custom hook that abstracts the problem away:

function useCallbackIfMounted(callback) {
  const isMountedRef = useIsMounted();
  return () => {
    if (isMountedRef.current) {
      callback();
    }
  }
}

function Button({callback}) {
  const mountedCallback = useCallbackIfMounted(callback);
  return <button onClick={mountedCallback}>Hallo</button>;
}

Using this would mean that fewer devs would have to figure out the ref hook bit. It doesn’t help for cases where you’re using a ref for something else, but is a nice solution to this specific problem.