Unexpected behavior with useState hook React

Question

I have some experience with class components in React, but am trying to learn hooks and functional components better.

I have the following code:

import React, { useState } from "react";
import { Button } from "reactstrap";
import StyleRow from "./StyleRow";

export default function Controls(props) {
  const [styles, setStyles] = useState([]);

  function removeStyle(index) {
    let newStyles = styles;
    newStyles.splice(index, 1);
    setStyles(newStyles);
  }

  return (
    <div>
      {styles}
      <Button
        color="primary"
        onClick={() => {
          setStyles(styles.concat(
            <div>
              <StyleRow />
              <Button onClick={() => removeStyle(styles.length)}>x</Button>
            </div>
          ));
        }}
      >
        +
      </Button>
    </div>
  );
}

The goal of this code is to have an array of components that have an "x" button next to each one that removes that specific component, as well as a "+" button at the bottom that adds a new one. The StyleRow component just returns a paragraph JSX element right now.

The unusual behavior is that when I click the "x" button by a row, it removes the element and all elements following it. It seems that each new StyleRow component that is added takes the state of styles at the moment of its creation and modifies that instead of always modifying the current styles state. This is different behavior than I would expect from a class component.

The freezing of state leads me to believe this has something to do with closures, which I don't fully understand, and I am curious to know what here triggered them. If anyone knows how to solve this problem and always modify the same state, I would greatly appreciate it.

Finally, I think this post on SO is similar, but I believe it addresses a slightly different question. If someone can explain how that answer solves this problem, of course feel free to close this question. Thank you in advance!

Answer 1

You are modifying the existing state of styles, so you will need to create a deep copy of the array first.

You can either write your own clone function, or you can import the Lodash cloneDeep function.

Add the following dependency to your package.json using:

npm install lodash

Also, you are passing the length of the array to the removeStyle function. You should be passing the last index which is length - 1.

// ...

import { cloneDeep } from 'lodash';

// ...

  function removeStyle(index) {
    let newStyles = cloneDeep(styles); // Copy styles
    newStyles.splice(index, 1);        // Splice from copy
    setStyles(newStyles);              // Assign copy to styles
  }

// ...

              <Button onClick={() => removeStyle(styles.length - 1)}>x</Button>
// ...

If you want to use a different clone function or write your own, there is a performance benchmark here:

"What is the most efficient way to deep clone an object in JavaScript?"

I would also move the function assigned to the onClick event handler in the button outside of the render function. It looks like you are calling setStyles which adds a button with a removeStyle event which itself calls setStyles. Once you move it out, you may be able to better diagnose your issue.

---

Update

I rewrote your component below. Try to render elements using the map method.

import React, { useState } from "react";
import { Button } from "reactstrap";

const Controls = (props) => {
  const [styles, setStyles] = useState([]);

  const removeStyle = (index) => {
    const newStyles = [...styles];
    newStyles.splice(index, 1);
    setStyles(newStyles);
  };

  const getChildNodeIndex = (elem) => {
    let position = 0;
    let curr = elem.previousSibling;
    while (curr != null) {
      if (curr.nodeType !== Node.TEXT_NODE) {
        position++;
      }
      curr = curr.previousSibling;
    }
    return position;
  };

  const handleRemove = (e) => {
    //removeStyle(parseInt(e.target.dataset.index, 10));
    removeStyle(getChildNodeIndex(e.target.closest("div")));
  };

  const handleAdd = (e) => setStyles([...styles, styles.length]);

  return (
    <div>
      {styles.map((style, index) => (
        <div key={index}>
          {style}
          <Button data-index={index} onClick={handleRemove}>
            &times;
          </Button>
        </div>
      ))}
      <Button color="primary" onClick={handleAdd}>
        +
      </Button>
    </div>
  );
};

export default Controls;

Answer 2

Let's try to understand what's going on here.

<Button
  color="primary"
  onClick={() => {
    setStyles(styles.concat(
      <div>
        <StyleRow />
        <Button onClick={() => removeStyle(styles.length)}>x</Button>
      </div>
    ));
  }}
>
  +
</Button>

First render: // styles = []

You add a new style. // styles = [<div1>]
The remove callback from the div is holding the reference to styles, whose length is now 0

You add one more style. // styles = [<div1>, <div2>]
Since div1 was created previously and didn't get created now, its still holding a reference to styles whose length is still 0.
div2 is now holding a reference to styles whose length is 1.

Now the same goes for the removeStyle callback that you have. Its a closure, which means it's holding a reference to a value of its outer function, even after the outer function has done executing. So when removeStyles is called for the first div1 the following lines will execute:

let newStyles = styles; // newStyles []

newStyles.splice(index, 1); // index(length) = 0;
// newStyles remain unchanged

setStyles(newStyles); // styles = [] (new state)

Now consider you have added 5 styles. So this is how the references will be held by each div

div1 // styles = [];
div2 // styles = [div1];
div3 // styles = [div1, div2];
div4 // styles = [div1, div2, div3];
div5 // styles = [div1, div2, div3, div4];

So what happens if you try to remove div3, the following removeStyly will execute:

let newStyles = styles; // newStyles = [div1, div2]
newStyles.splice(index, 1); // index(length) = 2;
// newStyles remain unchanged; newStyles = [div1, div2]

setStyles(newStyles); // styles = [div2, div2] (new state)

Hope that helps and addresses your concern. Feel free to drop any questions in the comments.

Here is a CodeSandbox for you to play around with and understand the issue properly.

Answer 3

I've added the most preferred way in a new answer as the previous one was becoming too long.

The explanation lies in my previous answer.

import React, { useState } from "react";
import { Button } from "reactstrap";

export default function Controls(props) {
  const [styles, setStyles] = useState([]);

  function removeStyle(index) {
    let newStyles = [...styles]
    newStyles.splice(index, 1);
    setStyles(newStyles);
  }

  const addStyle = () => {
    const newStyles = [...styles];
    newStyles.push({content: 'ABC'});
    setStyles(newStyles);
  };

  // we are mapping through styles and adding removeStyle newly and rerendering all the divs again every time the state updates with new styles.
  // this always ensures that the removeStyle callback has reference to the latest state at all times.
  return (
    <div>
      {styles.map((style, index) => {
        return (
          <div>
            <p>{style.content} - Index: {index}</p>
            <Button onClick={() => removeStyle(index)}>x</Button>
          </div>
        );
      })}
      <Button color="primary" onClick={addStyle}>+</Button>
    </div>
  );
}

Here is a CodeSandbox for you to play around.