0

I have the following scenario:

DisplayList component, which renders a list. The list record is passed in via props (this contains the list name, description, author etc), along with the initial items to be listed, and the current 'mode' (can be 'r', 'g' or 'a'). The component displays the list information, and then renders a DisplayItem component for each list item. The DisplayItem component also takes a callback function, so that when the user clicks on an item in the list, it becomes 'tagged' with the current mode (ie the 'mode' property of the item changes).

const DisplayList = (props) => {

  // Get list object (contains list name etc)
  // and current mode (r, a or g)
  const { list, currentMode } = props
  
  // Array of items to show
  // This is an array of objects, with each object representing a single item
  // eg {name: itemname, mode: r}
  const [ items,  setItems] = useState(props.items)

  // Callback function - triggered when the rendered DisplayItem is clicked
  // This changes the 'mode' of the item to the current mode
  function handleItemClick(index) {
      items[index].mode = currentMode
      setItems(items) // At this point the list of items should re-render
    }
  }

  return (
    <div className="displayItem">
      {
        items.map(item => <DisplayItem key={uuid()} item={item} handleCellClick={handleItemClick} />)
      }
    </div>
  )

}

The problem I'm having is that clicking an item seems to trigger the handleItemClick function to update the itemList, but when it gets to setItems, it doesn't re-render.

halfer
  • 19,824
  • 17
  • 99
  • 186
Sharon
  • 3,471
  • 13
  • 60
  • 93
  • 1
    Not a direct answer, but generating a uuid every render to use as the key is pointless. If you don't have a unique identifier to reuse every render use the array index. – Brian Thompson Oct 28 '20 at 20:49
  • 1
    The more direct answer: `items[index].mode = currentMode` mutates state and passes the original array reference to the updater function, meaning react will not see it as a change no matter how many nested values you update. Create a new copy of state before mutating, then give the updater function the *new* array. – Brian Thompson Oct 28 '20 at 20:51
  • 1
    Brian is correct you just need to make a new array. But you should use an uuid, using the array index is a bad practice. It will slow down your application and cause issues, because you can't guarantee that an item will always have the same index. Best practice would be to give each item a unique id and use that, but uuid is better than index. https://stackoverflow.com/questions/46735483/error-do-not-use-array-index-in-keys – nzajt Oct 28 '20 at 20:57
  • 1
    @nzajt no it is absolutely not a better practice. Generating a uuid as the key during a render makes no sense.. React uses the key to distinguish component instances for subsequent updates, unmounts, etc. This means if an element gets a new key every render, react will treat it as a **new** component instance and will **re-mount** the element. If your element is a component with state, you **will** find bugs as the state will reset and lifecycle methods will restart. **If** you do not have a unique identifier, you may use the index. This is what react does by default, however it is not ideal. – Brian Thompson Oct 28 '20 at 21:00
  • 1
    See [this answer](https://stackoverflow.com/a/52706585/9381601) for the correct way to use a uuid as the key, while not getting the performance hit from generating it during render. – Brian Thompson Oct 28 '20 at 21:05
  • Thank you, that's really helpful. I tried using the index as the key, thinking that should stay the same, but when I do that, the re-render doesn't happen. If I use uuid, it works (once I create a new copy of state and passing that to the updater function as explained above by @BrianThompson). – Sharon Oct 29 '20 at 10:36

1 Answers1

1

You are not passing the index to your callback.

try this:

items.map({item, index} => <DisplayItem key={uuid()} item={item} handleCellClick={() => handleItemClick(index)} />)

edited as pointed by Brian Thompson at the comments

hgodinho
  • 70
  • 2
  • 8
  • 1
    You're correct that they aren't passing the index, but this code will cause an infinite render loop. It should be `handleCellClick={() => handleItemClick(index)}` – Brian Thompson Oct 28 '20 at 20:54
  • 1
    Hi @BrianThompson, thanks. Can you elaborate on this? – hgodinho Oct 28 '20 at 21:21
  • 2
    Sure. JSX is just a [nicer way of writing function calls](https://reactjs.org/docs/react-api.html#createelement). When it transpiles to JavaScript, it will look like this: `React.createElement(DisplayItem, { key: uuid(), item: item, handleCellClick: handleItemClick(index) })`. Now it may be hard to see immediately in a comment, but what this does is assign `handleCellClick` the **return value** of the function call, **not** the function reference (to be called later on a click event). So what you want to do is pass it an inline function **reference** like in my comment above. – Brian Thompson Oct 28 '20 at 21:31
  • 2
    The reason it would cause an infinite loop in this case, is that `handleItemClick` updates state. State updates trigger a re-render, and we're calling the function every render, and there we have our infinite loop. – Brian Thompson Oct 28 '20 at 21:34
  • 1
    Nice! I will edit my answer with your suggest. Thanks. – hgodinho Oct 28 '20 at 21:39
  • Thanks. index is stored within the item object, so that's why I'm not passing it in. Changing the function call seems to be working though. – Sharon Oct 29 '20 at 10:29