CODEWITHSUNDEEP
reactjsreact-hooksuse-effect
Luke
Luke

Reputation: 19991

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

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.

Upvotes: 0

Views: 1196

Answers (3)

Daniel
Daniel

Reputation: 15423

So you would want to throw that function inside of the useEffect() hook and avail yourself of useRef like so:

import React, { useEffect, useState, useRef } from 'react';

const MyComponent = ({ options, selected }) => {
  const [open, setOpen] = useState(false);
  const ref = useRef();

  useEffect(() => {
  const setupDocumentClickEffect = (event) => {
    // this if conditional logic assumes React v17
    if (ref.current && ref.current.contains(event.target)) {
      return;
    }
    setOpen(false);
  };

   document.body.addEventListener('click', setupDocumentClickEffect);

   return () => {
     document.body.removeEventListener('click', setupDocumentClickEffect);
   };
  }, []);
}

So since it's a menu, I imagine you build your list via a map() function somewhere that in this example, I am calling options which is why you see it passed as props in your MyComponent and you want to render that list of options from the menu:

import React, { useEffect, useState, useRef } from 'react';

const MyComponent = ({ label, options, selected, onSelectedChange }) => {
  const [open, setOpen] = useState(false);
  const ref = useRef();

  useEffect(() => {
  const setupDocumentClickEffect = (event) => {
    // this if conditional logic assumes React v17
    if (ref.current && ref.current.contains(event.target)) {
      return;
    }
    setOpen(false);
  };

   document.body.addEventListener('click', setupDocumentClickEffect);

   return () => {
     document.body.removeEventListener('click', setupDocumentClickEffect);
   };
  }, []);

  const renderedOptions = options.map((option) => {
    if (option.value === selected.value) {
      return null;
    }

    return (
      <div 
       key={option.value}
       className="item"
       onClick={() => {
        onSelectedChange(option);
       }}
      >
       {option.label}
      </div>
    );
  });

  return (
    <div ref={ref} className="ui form">
     // the rest of your JSX code here including
     // renderedOptions below
     {renderedOptions}
    </div>
  );
};

export default MyComponent;

So I added some props to your MyComponent and also showed you how to implement that useRef which will be important in pulling this off as well.

Upvotes: 0

Cory Harper
Cory Harper

Reputation: 1055

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]);

}

Upvotes: 2

Adam Stox
Adam Stox

Reputation: 457

I suspect it's because you're calling setupDocumentClickEffect(close) immediately inside of useEffect(). Using a deferred call like useEffect(() => setupDocumentClickEffect(close), []) is what you want.

It might not break the useEffect hook, but it would be better practice to incorporate your if(open) within setupDocumentClickEffect() instead of wrapping your hook in it.

Upvotes: -1

Related Questions