17

I have a simple list of student objects with name and their scores in state.

Their name is bound to <b>{student.name}</b> and their score is bound to

<input type="text" defaultValue={student.score}/>

Whenever I want to delete the first student from this list and re-rendering the component by calling set state.

The input tag of the second student showing the score of the first student instead of having its own. Why this happening where I am doing it wrong ??

Here is my code jsbin

class App extends React.Component{
  constructor(){
    super();
    this.state ={
      students:[{name:"A",score:10},{name:"B",score:20},{name:"C",score:30}]
    }
  }
  
  onDelete(index){
    this.state.students.splice(index,1);
    this.setState(this.state);
  }
  
   render(){
     return(
       <div>
         {this.state.students.map((student,index)=>{
              return(
                <div key={index}>
                    <b>{student.name}</b> - <input type="text" defaultValue={student.score}/>
                     <button onClick={this.onDelete.bind(this,index)}>delete</button>
                </div>                
              )
         })}
       </div>
     )
   }
}


ReactDOM.render(<App/>,document.getElementById("main"));
Nguyễn Văn Phong
  • 13,506
  • 17
  • 39
  • 56
manas
  • 6,119
  • 10
  • 45
  • 56

3 Answers3

42

It's because you're using key={index} instead of a value unique to the student.

When the array is modified the students after the index removed will have the wrong key and React will not register a key change to rerender with the updated data.

You should instead use something like this...

<div key={student.name}>

assuming that student.name is unique.

BradByte
  • 11,015
  • 2
  • 37
  • 41
  • Thanks a lot . But why it works with any other tag other than input type="text" ?? – manas Apr 26 '17 at 19:02
  • Any other tag than `input` (so a `span`) or another input type? – BradByte Apr 26 '17 at 19:04
  • 1
    `defaultValue` doesn't update when it changes... you would need to trash the element and regenerate it (i.e. change the `key`).. whereas another element would probably work ok. If you used `value` it might work.. but you would only have a read only input unless you give it an `onChange` handler. – BradByte Apr 26 '17 at 19:05
  • I generated random 6 digit numbers as ID's for the objects I was initializing, set those as the key when iterating, and that resolved my problem. – Nik Hammer-Ellis Feb 12 '19 at 19:03
  • this answer helps a lot! – John Aldrin Jul 23 '22 at 10:48
3

It is always better to use unique id as a key instead of index. If your JSON does not provide a unique Id, you can use something like uuid npm module which generates it for you. This is the link https://www.npmjs.com/package/uuid.

You can then import it and use as below

import { v4 } from 'uuid'; //there are 5 versions. You can use any.

then use it to generate uique Id by calling the v4 function like below

id={v4()}
Aswin K
  • 333
  • 3
  • 12
  • Generating a unique id when not provided by the api is a good idea, but you should do it before rendering. If you do it during rendering like you are suggesting in your answer, you are essentially constantly recreating ids over and over again and so every time, there's an update, you get a new id. This results in unnecessary renders and if you have an input field, it will lose focus on every character input. – earthling Nov 04 '17 at 22:54
  • I meant to generate id, thanks for catching. edited my answer. – Aswin K Nov 05 '17 at 23:34
  • Bad idea, you will destroy child component state if you keep assigning it new keys. – lawrence-witt May 11 '21 at 13:11
0

Best practice

To be honest, you shouldn't mutate state data directly. You should clone then do your task like this

onDelete(index) {
    const newStudents = [...this.state.students];
    newStudents.splice(index, 1);
    this.setState({ students: newStudents });
}

Why can't I directly modify a component's state, really?

The previous state will be polluted with your mutation. Due to which, the shallow compare and merge of two states will be disturbed or won't happen, because you'll have only one state now. This will disrupt all the React's Lifecycle Methods.


Your problem & solution

  1. In case that you use the input readOnly, you should change from defaultValue to value then your code work well.
<input type="text" value={student.score} readOnly />

class App extends React.Component {
  constructor() {
    super();
    this.state = {
      students: [
        { name: "A", score: 10 },
        { name: "B", score: 20 },
        { name: "C", score: 30 }
      ]
    };
  }

  onDelete(index) {
    const newStudents = [...this.state.students];
    newStudents.splice(index, 1);
    this.setState({ students: newStudents });
  }

  render() {
    return (
      <div>
        {this.state.students.map((student, index) => {
          return (
            <div key={index}>
              <b>{student.name}</b> -{" "}
              <input type="text" value={student.score} readOnly />
              <button onClick={this.onDelete.bind(this, index)}>delete</button>
            </div>
          );
        })}
      </div>
    );
  }
}

ReactDOM.render(<App/>, document.getElementById('app'));
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/17.0.1/umd/react.production.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/17.0.1/umd/react-dom.production.min.js"></script>

<div id="app"></div>
  1. Otherwise, you should provide the unique key. If student.name is not unique, you can randomly GUID like this.
const getGUID = () => "id" + Math.random().toString(16).slice(2);
>  <div key={getGUID()}>

const getGUID = () => "id" + Math.random().toString(16).slice(2);

class App extends React.Component {
  constructor() {
    super();
    this.state = {
      students: [
        { name: "A", score: 10 },
        { name: "B", score: 20 },
        { name: "C", score: 30 }
      ]
    };
  }

  onDelete(index) {
    const newStudents = [...this.state.students];
    newStudents.splice(index, 1);
    this.setState({ students: newStudents });
  }

  render() {
    return (
      <div>
        {this.state.students.map((student, index) => {
          return (
            <div key={getGUID()}>
              <b>{student.name}</b> -{" "}
              <input type="text" defaultValue={student.score} />
              <button onClick={this.onDelete.bind(this, index)}>delete</button>
            </div>
          );
        })}
      </div>
    );
  }
}

ReactDOM.render(<App/>, document.getElementById('app'));
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/17.0.1/umd/react.production.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/17.0.1/umd/react-dom.production.min.js"></script>

<div id="app"></div>

However, in this case, Unstable keys will cause harmful performance because component instances - Virtual DOM & DOM node - actual DOM will always unnecessary recreated.

In short, depends on your data and behavior, you can choose the correct way to complete it.

Nguyễn Văn Phong
  • 13,506
  • 17
  • 39
  • 56
  • Generating a new key on every render will not only cause performance issues but introduce bugs. I would not recommend it as a solution at all. – lawrence-witt May 11 '21 at 13:09