Why is React useEffect clean-up function being run right after the useEffect callback, and then never again?

Question

I have a small React app with a component that has a button that opens a small menu, and I'd like it to close the menu when the user clicks anywhere outside the component.

function setupDocumentClickEffect(onClick = (() => {})) {
  console.log('Add doc click');
  document.addEventListener('click', onClick);
  return () => { // Clean-up
    console.log('Remove doc click');
    document.removeEventListener('click', onClick);
  };
}

function MyComponent() {
  const [open, setOpen] = useState(false);

  // Set up an effect that will close the component if clicking on the document outside the component
  if (open) {
    const close = () => { setOpen(false); };
    useEffect(setupDocumentClickEffect(close), [open]);
  }

  const stopProp = (event) => { event.stopPropagation(); };
  const toggleOpen = () => { setOpen(!open); };
  // ...
  // returns an html interface that calls stopProp if clicked on the component itself,
  // or toggleOpen if clicked on a specific button.
}

When the component is first opened, it will run both the callback and the cleanup immediately. Console will show: Add doc click and Remove doc click. If the component is closed and then re-opened, it acts as expected with just Add doc click, not running clean-up... but then clean-up is never run again.

I suspect I'll have to re-structure this so it doesn't use if (open), and instead runs useEffect each time? But I'm not sure why the clean-up runs the way it does.

Answer 1

A few things are wrong here. The first argument to a useEffect should be a callback function, which you're returning from setupDocumentClickEffect, this means that the return value of setupDocumentClickEffect(close) will just be run immediately on mount, and never again.

It should look more like this:

useEffect(() => {
  if (!open) {
    return;
  }

  console.log('Add doc click');
  document.addEventListener('click', close);
  return () => { // Clean-up
    console.log('Remove doc click');
    document.removeEventListener('click', close);
  };
}, [open]);

The other thing that is wrong here is that you are breaking the rules of hooks: https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level

You should not define a hook in a conditional.

EDIT

To elaborate on what is happening in your current useEffect, it basically boils down to if you wrote something like this:

if (open) {
    const close = () => { setOpen(false); };
    console.log('Add doc click');
    document.addEventListener('click', close);
    
    useEffect(() => {
      console.log('Remove doc click');
      document.removeEventListener('click', close);
    }, [open]);

}