0

I have the following code:

    function App() {

      // array to store 3 numbers, all initialized to 0
      const [counter, setCounter] = React.useState<number[]>([0, 0, 0]);

      // increments counter[i] by 1
      const increment = (i: number) => {
        let newCount = [...counter];
        newCount[i] += 1;  
        setCounter(newCount);
      }

      return (
        <div className="App">
          <header className="App-header">
            <p>            
              count: {counter[0]} {counter[1]} {counter[2]}
            </p>
            <button onClick={() => {
              [0, 1, 2].map((i) => {  
                // pretend that setTimeout is making some web request
                setTimeout(() => increment(i), 1000); 
              });
            }} >Increment All</button>
          </header>
        </div>
      );
    }

When I click on the button, I want it to increment all 3 numbers in the array, but it looks like only the last setTimeout call will take effect (and thus only one of the values get incremented).

I think I understand why that is (the counter value is constant within the react onClick event handler call), but I don't know how best to structure this code to avoid this issue. Do I have to buffer the increments and "commit" the changes all at once via one call to setCounter? Is there a better way?

CBH
  • 23
  • 4

2 Answers2

2

To add to JMadeleine's answer, you can get around that issue by putting some scope around the setTimeout call in the form of a function.

[0,1,2].map((i) => {
  (function() {
    setTimeout(() => increment(i),1000);
  })() // <- call that new function
});

See these posts for some helpful info

why does javascript setTimeout() not work in a loop?

https://coderwall.com/p/_ppzrw/be-careful-with-settimeout-in-loops

D. Smith
  • 739
  • 5
  • 9
1

Putting a console.log in your increment function shows the error:

  const increment = (i: number) => {
    let newCount = [...counter];
    // log values here
    console.log(i, counter)
    newCount[i] += 1;  
    setCounter(newCount);
  }

The log reads:

"0, [0, 0, 0]"
"1, [0, 0, 0]"
"2, [0, 0, 0]"

The value of counter is always [0, 0, 0]. This is caused by closures.

When you call setTimeout(() => increment(i), 1000) for each of the values in [0, 1, 2], JavaScript creates three functions in memory:

() => increment(0)

() => increment(1)

() => increment(2)

These functions will execute after 1 second.

However, as these need to use the increment function, the increment function is also 'copied' into memory.

The increment function also uses the counter variable, so the counter variable is also copied into memory - with its current value [0, 0, 0].

This is the value that will be used in all three functions.

This is why you are seeing only the last value increment, because you're essentially doing this:

 setCounter([1, 0, 0])
 setCounter([0, 1, 0])
 setCounter([0, 0, 1])

One solution would be to not use setTimeout at all, but a more robust solution is to provide a callback function to setCounter:

const increment = (i: number) => {
  setCounter(prev => {
    let newCount = [...prev]
    newCount[i] += 1
    return newCount
  })
}

The rule is, if the new state values depends on the old state value (e.g., when incrementing a number, the new value is determined by the old value) then you should pass a callback to set state.

You cannot rely on counter being an up to date value.


Please see D. Smith's answer for some useful links relating to closures and setTimeout.

JMadelaine
  • 2,859
  • 1
  • 12
  • 17