Why does setState in a loop insert repeated value

Question

I have two logically dependent HTML select components.

The first one represents Districts list and the second one represents corresponding Subdistricts.

When a District is selected, the Subdistricts option array should be altered to represent Subdistricts from the selected District.

Here is how they are represented in components render method:

  <div style={{display: "inline-block", marginRight: "20px"}}>
      <h style={{fontSize: "20px"}}>District</h>
      <br/>
      <select id="districts-select-list" onChange={this.updateDistrictData}>
          {this.state.districtsSelectOptionsArrayState}
      </select>
  </div>

  <div style={{display: "inline-block"}}>
      <h style={{fontSize: "20px"}}>Subdistrict</h>
      <br/>
      <select id="subdistricts-select-list">
          {this.state.subdistrictsSelectOptionsArrayState}
      </select>
      {this.state.subdistrictsSelectOptionsArrayState}
  </div>

As you see options are state dependent.

Here is how update the data:

updateDistrictData(e) {
    this.setState({subdistrictsSelectOptionsArrayState : []});

    var categoryList = document.getElementById("districts-select-list");
    var selectedDistrictId = categoryList.options[categoryList.selectedIndex].value;

    if(selectedDistrictId == undefined) {
        return;
    }

    var currentSubdistrictList = this.subdistrictDataArray[selectedDistrictId];

    if(currentSubdistrictList != undefined) {
        var currentSubdistrictListLength = currentSubdistrictList.length;

        if(
            currentSubdistrictListLength == undefined ||
            currentSubdistrictListLength == 0
        ) {
            return;
        }

        for(var index = 0; index < currentSubdistrictListLength; index++) {
            var currentDistrictObject = currentSubdistrictList[index];

            if(currentDistrictObject != undefined) {
                var currentSubdistrictId = currentDistrictObject["id"];
                var currentSubdistrictName = currentDistrictObject["name"];

                console.log("SUBDISTRICT NAME IS : " + currentSubdistrictName);

                var currentSubdistrictOption = (
                    <option value={currentSubdistrictId}>
                        {currentSubdistrictName}
                    </option>
                );

                this.setState(prevState => ({
                    subdistrictsSelectOptionsArrayState:[
                        ...prevState.subdistrictsSelectOptionsArrayState, 
                        (
                            <option value={currentSubdistrictId}>
                                {currentSubdistrictName}
                            </option>
                        )
                    ]
                }));

            }

        }

    }

}

I call updateDistrictData method after retrieving subdistricts from server and in District select component's onChange method.

When the page is loaded for the first time, the districts and corresponding subdistricts are altered correctly.

But when I change district afterwards using the District select component itself, the subdistricts select component is populated with repetous subdistrict option repeated as many times as the number of subdisctricts in the current district.

What am I doing wrong?

Answer 1

The problem is caused by the use of vars (currentSubdistrictId and currentSubdistrictName) in a closure (the setState callBack).

=> because of the var declaration, the last value taken by currentSubdistrictId and currentSubdistrictName were used for all options.

Closures are really tricky in js when used with vars (sort of global scope).

Since you're using es6, you should properly use let (modified within a block like index in the for loop) and const (set only once when declared in a block) variables declaration and never use var (sort of global scope).

class App extends React.Component {
    districtDataArray = [
        { name: 'A', id: 0 },
        { name: 'B', id: 1 },
        { name: 'C', id: 2 },
    ]
    subdistrictDataArray = [
        [
            { name: 'AA', id: 0 },
            { name: 'AB', id: 1 },
            { name: 'AC', id: 2 },
        ],
        [
            { name: 'BA', id: 0 },
            { name: 'BB', id: 1 },
            { name: 'BC', id: 2 },
        ],
        [
            { name: 'CA', id: 0 },
            { name: 'CB', id: 1 },
            { name: 'CC', id: 2 },
        ],
    ]

    state = {
        districtsSelectOptionsArrayState: this.districtDataArray.map(d => (
            <option value={d.id}>
                {d.name}
            </option>
        )),
        subdistrictsSelectOptionsArrayState: [],
    }

    constructor(props) {
        super(props);
        this.updateDistrictData = this.updateDistrictData.bind(this);
    }

    updateDistrictData(e) {
        this.setState({subdistrictsSelectOptionsArrayState : []});

        const categoryList = document.getElementById("districts-select-list");
        const selectedDistrictId = categoryList.options[categoryList.selectedIndex].value;

        if(!selectedDistrictId) {
            return;
        }

        const currentSubdistrictList = this.subdistrictDataArray[selectedDistrictId];

        if(currentSubdistrictList) {
            const currentSubdistrictListLength = currentSubdistrictList.length;

            if(!currentSubdistrictListLength) {
                return;
            }

            for(let index = 0; index < currentSubdistrictListLength; index++) {
                // use const for block level constant variables
                const currentDistrictObject = currentSubdistrictList[index];

                if(currentDistrictObject) {
                    // use const for block level constant variables
                    const currentSubdistrictId = currentDistrictObject["id"];
                    const currentSubdistrictName = currentDistrictObject["name"];

                    console.log("SUBDISTRICT NAME IS : " + currentSubdistrictName);

                    const currentSubdistrictOption = (
                        <option value={currentSubdistrictId}>
                            {currentSubdistrictName}
                        </option>
                    );

                    this.setState(prevState => ({
                        subdistrictsSelectOptionsArrayState:[
                            ...prevState.subdistrictsSelectOptionsArrayState, 
                            (
                                <option value={currentSubdistrictId}>
                                    {currentSubdistrictName}
                                </option>
                            )
                        ]
                    }));

                }

            }

        }

    }
    render() {
    
        return (
            <div>
                <div style={{display: "inline-block", marginRight: "20px"}}>
                    <h style={{fontSize: "20px"}}>District</h>
                    <br/>
                    <select id="districts-select-list" onChange={this.updateDistrictData}>
                        {this.state.districtsSelectOptionsArrayState}
                    </select>
                </div>

                <div style={{display: "inline-block"}}>
                    <h style={{fontSize: "20px"}}>Subdistrict</h>
                    <br/>
                    <select id="subdistricts-select-list">
                        {this.state.subdistrictsSelectOptionsArrayState}
                    </select>
                    {this.state.subdistrictsSelectOptionsArrayState}
                </div>
            </div>
        );
    }
}

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" />

Also, the way you are updating the state in updateDistrictData is very inefficient (n + 1 setStates, n in a loop, n being the number of subdistricts).

You should compute the final state in a variable and set it all at once when computation is done.

While my implementation explains what was wrong with your code without altering it too much, Jared's answer below is a very good example of how it could be done cleaner.

Answer 2

This should solve the original problem, but this should hopefully solve a lot of other problems...

Your display code should look something like this:

  <div style={{ display: "inline-block", marginRight: "20px" }}>
    <h style={{ fontSize: "20px" }}>District</h>
    <br />
    <select id="districts-select-list" onChange={this.updateDistrictData}>
      {this.state.districts.map(({ name, id }) => (
        <option value={id} key={id}>{name}</option>
      ))}
    </select>
  </div>

  <div style={{ display: "inline-block" }}>
    <h style={{ fontSize: "20px" }}>Subdistrict</h>
    <br />
    <select id="subdistricts-select-list">
      {
        this.state
          .subdistricts
          .filter(({ districtId }) => districtId === this.state.selectedDistrictId)
          .map(({ id, name }) => <option value={id} key={id}>{name}</option>)
      }
    </select>
  </div>

And your update code now looks like this:

updateDistrictData (e) {
  this.setState({ selectedDistrictId: e.target.value });
}

There's no point in storing all of those JSX react nodes in state, as long as you use the unique id for the key property React won't do unnecessary re-renders.

I would further recommend that you move the lists out of state entirely, and pass them in as props from a stateful parent component. Generally it's better to have components that display information to the user to be totally stateless and have the manipulation of state higher up the chain.