Ven Nilson
Ven Nilson

Reputation: 1019

useEffect is not removing events correctly

I'm facing a problem there is <App/> component which renders a child i.e. <Button/> component. The <Button/> component has 2 props one is a boolean value and the other is function. A backdrop will show when the user clicks the button, backdrop is just a div that hides on the user click.

When the boolean prop is changed, the <Button/> component dynamically adds the event listeners and removes the listeners when the user clicks anywhere on the document.

I successfully added this functionality with class-based lifecycle hooks but facing a problem when replacing the component with React hooks.

I know there is a hook named useEffect which can replace the behavior of componentDidMount, componentWillUnmount and componentDidUpdate but my implementation of useEffect is not removing listeners based on the prop changed. Even when the backdrop is hidden, listeners are working without clicking a button and why the linter is complaining.

React Hook useEffect has missing dependencies: 'addEvents' and 'removeEvents'. Either include them or remove the dependency array. (react-hooks/exhaustive-deps).

How can I fix this behavior and make this component like a class <Button /> component?

Functionality with class component:

// CSS styles for backdrop
const customStyles = {
  backgroundColor: "rgba(0,0,0,.5)",
  position: "absolute",
  top: 0,
  left: 0,
  right: 0,
  bottom: 0
};

// Button class component
class Button extends React.Component {
  // Return the function
  toggle = event => {
    return this.props.toggle(event);
  };

  // Attach Listener to the document object
  handleDocumentClick = event => {
    this.toggle(event);
  };

  // Add listeners to the document object
  addEvents = () => {
    ["click", "touchstart"].forEach(event =>
      // Event propogate from body(root) element to eventTriggered element.
      document.addEventListener(event, this.handleDocumentClick, true)
    );
  };

  // remove listeners from the document object
  removeEvents = () => {
    ["click", "touchstart"].forEach(event =>
      document.removeEventListener(event, this.handleDocumentClick, true)
    );
  };

  // Add or remove listeners when the prop changes
  manageProp = () => {
    if (this.props.open) {
      this.addEvents();
    } else {
      this.removeEvents();
    }
  };

  componentDidMount() {
    this.manageProp();
  }

  componentWillUnmount() {
    alert("Button cleanup");
    this.removeEvents();
  }

  componentDidUpdate(prevProps) {
    if (this.props.open !== prevProps.open) {
      this.manageProp();
    }
  }

  render() {
    return <button onClick={this.toggle}>Button</button>;
  }
}

// App class component
class App extends React.Component {
  state = {
    open: false
  };

  toggle = () => {
    alert("Button is clicked");
    this.setState({
      open: !this.state.open
    });
  };

  render() {
    return (
      <div className="app">
        {/* Class-based Button Component */}
        <Button open={this.state.open} toggle={this.toggle} />
        {/* Backdrop */}
        {this.state.open && <div style={customStyles} />}
      </div>
    );
  }
}

// Render it
ReactDOM.render(<App />, document.getElementById("root"));
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/16.6.3/umd/react.production.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/16.6.3/umd/react-dom.production.min.js"></script>
<div id="root"></div>

Component with react hook:

import React, { useEffect } from "react";

const Button = props => {
  const toggle = event => {
    return props.toggle(event);
  };

  // Attach this to the document object
  const handleDocumentClick = event => {
    toggle(event);
  };

  // Add event listeners
  const addEvents = () => {
    ["click", "touchstart"].forEach(event =>
      document.addEventListener(event, handleDocumentClick, true)
    );
  };

  // Remove event listeners
  const removeEvents = () => {
    ["click", "touchstart"].forEach(event =>
      document.removeEventListener(event, handleDocumentClick, true)
    );
  };

  // Add or remove listeners based on the state changes
  const manageProp = () => {
    if (props.open) {
      addEvents();
    } else {
      removeEvents();
    }
  };

  // Mount, Unmount & DidUpdate
  useEffect(() => {
    manageProp();
    // Unmount
    return () => {
      alert("Button cleanup");
      removeEvents();
    };
  }, [props.open]);

  // Render it
  return <button onClick={toggle}>Button</button>;
};

export default Button;

Sandbox link

Upvotes: 2

Views: 2858

Answers (2)

Agney
Agney

Reputation: 19194

The function returned from useEffect every time any of the dependencies change. In your case, that dependency is props.open.

When you call removeEvents from this function, you will add the events twice by virtue of props.open being true. Instead what you can do is to remove the event everytime the prop changes:

useEffect(() => {
    // Add or remove listeners based on the state changes
    const manageProp = () => {
      if (props.open) {
        addEvents();
      } else {
        removeEvents();
      }
    };
    // mount
    manageProp();
    // unmount
    return () => {
      removeEvents();
    };
  }, [props.open]);

Codesandbox

Just move addEvents and removeEvents into the useEffect to get rid of the warning

Upvotes: 1

giuseppedeponte
giuseppedeponte

Reputation: 2391

I have done a little bit of clean up of your code, but the main point is you have to store props.open in a locally scoped constant before using it in your useEffect hook:

import React, { useEffect } from "react";

const Button = props => {
  const isOpen = props.open;

  // Add event listeners
  const addEvents = () => {
    ["click", "touchstart"].forEach(event =>
      document.addEventListener(event, props.toggle, true)
    );
  };

  // Remove event listeners
  const removeEvents = () => {
    ["click", "touchstart"].forEach(event =>
      document.removeEventListener(event, props.toggle, true)
    );
  };

  // Mount, Unmount & DidUpdate
  useEffect(() => {
    // Add or remove listeners based on the state changes
    const manageProp = () => {
      if (isOpen) {
        addEvents();
      } else {
        removeEvents();
      }
    };
    // mount
    manageProp();
    // unmount
    return manageProp;
  }, [isOpen]);

  // Render it
  return <button onClick={props.toggle}>Button</button>;
};

export default Button;

Forked sandbox here
And some reading for you here

Upvotes: 0

Related Questions