React JS state updates for collections of objects

Question

I'm pretty new to React, and I'm trying to practice by building a simple notes app. As far as I can tell it's going great, but! I read that state should not be updated manually, so I'm copying my state array and filtering out a result for a removal operation.

But it fails! Rather, if I console log, it correctly removes the to-be-deleted element from the state array, however, when I call setState() on the copy to update my view, the list is wrong!

For some reason my React list is always removing the last element visually from the page, and appears then out of sync with my state.

The app itself is a Form container of sorts with a nested list and list-item component, which use props from the form class to manage.

What am I doing wrong?

Form Class

class NotesForm extends Component {
  constructor(props) {
    super(props);

    const list = [
     { text: "Build out UI" },
     { text: "Add new note" },
     { text: "delete notes" },
     { text: "edit notes" }
   ];

    this.state = {
      'notes': list
    };

    // this.notes = list;
    this.handleSubmit = this.handleSubmit.bind(this);
    this.deleteNote = this.deleteNote.bind(this);
  }

  handleSubmit(e) {
    e.preventDefault();
    if (this.input.value.length === 0) { return; }

    this.state.notes.push({text: this.input.value});
    this.setState({ notes: this.state.notes });

    this.input.value = "";
  }

  // BUG - deletes WRONG note!!
  deleteNote(note) {
    console.log({'DELETE_NOTE': note.text})
    // var list = _.clone(this.state.notes);
    var list = [...this.state.notes];

    var filteredNotes = _.filter(list, function(n) {
      return (n.text !== note.text);
    })

    console.log({
      'list': list,
      'filteredNotes': filteredNotes
    })

    this.setState({ notes: filteredNotes });
  }

  render() {
    return (
      <div className="row notes-form">
        <div className="col-xs-12">
          <form onSubmit={this.handleSubmit}>
            <input type="text" className="new-note-input" ref={(input) => this.input = input} />
            <br />
            <button className="add-btn btn btn-info btn-block" type="button" onClick={this.handleSubmit}>Add</button>
            <br />
            <NotesList notes={this.state.notes} deleteNote={this.deleteNote} />
          </form>
        </div>
      </div>
    );
  }
}

List Class

class NotesList extends Component {
  constructor(props) {
    super(props);
  }

  render() {
    return (
      <ul className="notes-list">
        {this.props.notes.map((n, index) => <NotesListItem key={index} note={n} deleteNote={this.props.deleteNote} />)}
      </ul>
    );
  }
}

List Item Class

class NotesListItem extends Component {
  constructor(props) {
    super(props);

    this.state = {
      'text': props.note.text
    };

    this.delete = this.delete.bind(this);
  }

  delete() {
    this.props.deleteNote(this.props.note);
  }

  render() {
    return (

      <li className="notes-list-item">
        <span className="item-text">{this.state.text}</span>
        <div className="notes-btn-group btn-group" role="group">
          <button className="delete-btn btn btn-danger" type="button" onClick={this.delete}>&times;</button>
        </div>
      </li>

    );
  }
}

Answer 1

Try using something like a unique id instead of index as the key for each NotesListItem in NotesList. See this related question (maybe a duplicate actually):

import React, { Component } from 'react';
import NotesListItem from './NotesListItem';

class NotesList extends Component {
  constructor(props) {
    super(props);
  }

  render() {
    return (
      <ul className="notes-list">
        {this.props.notes.map((n, index) => <NotesListItem key={n.id} note={n} deleteNote={this.props.deleteNote} />)}
      </ul>
    );
  }
}

export default NotesList;

You can use something like uuid to generate a "unique" id. There are many ways you could generate a unique key, but it depends on your data structure. Also using a unique id and filtering based on the id, can help avoid a situation where two notes in the array have the same text as filtering based on the text value would delete both of them.

import uuidv1 from 'uuid/v1';

// ...

handleSubmit(e) {
  e.preventDefault();
  if (this.input.value.length === 0) { return; }

  this.state.notes.push({id: uuidv1(), text: this.input.value});
  this.setState({ notes: this.state.notes });

  this.input.value = "";
}

I only suggest to use something like this as it's possible your text could be duplicated. You could probably even get away with using something like:

{this.props.notes.map((n, index) => <NotesListItem key={index + n.text} note={n} deleteNote={this.props.deleteNote} />)}

Also, you shouldn't be directly mutating state like this.state.notes.push({text: this.input.value});. Try something like this instead:

handleSubmit(e) {
  e.preventDefault();
  if (this.input.value.length === 0) { return; }

  const note = { id: uuidv1(), text: this.input.value };
  const notes = [...this.state.notes, note];

  this.setState({ notes });

  this.input.value = "";
}

Also, I'd avoid using ref for handling controlled inputs, especially to set value. Why not create a property on state that handles the value of the input in combination with a simple onChange event handler. This would be in line with the React Forms documentation and the "standard" React way of handling input value updates:

handleChange(e) {
  this.setState({ text: e.target.value });
}

handleSubmit(e) {
  e.preventDefault();
  if (this.state.text.length === 0) { return; }

  const note = { id: uuidv1(), text: this.state.text };
  const notes = [...this.state.notes, note];

  this.setState({ text: '', notes });
}

render() {
  // ...
  <input type="text" className="new-note-input" value={this.state.text} onChange={this.handleChange} />
  // ...
}

Here is an example in action.

The other answer may be enough to resolve your issue. I'd recommend to review the following article mentioned/linked in the React Keys documentation discuss the potential negative impacts of using an index as a key.

Hopefully that helps!

Answer 2

The constructor of a Component only runs once. React will reuse component instances passing them new props. The problem here is that NodeListItem caches the text of the note in its own local state and uses that text in the render method. When its Parent passes a new note to it via the props, it does not use it. It uses the state which is now stale.

Child components should usually use data from props passed in by the Parent.

class NotesListItem extends Component {
  constructor(props) {
    super(props);

    // The problem is this statement here
    this.state = {
      'text': props.note.text
    };

    this.delete = this.delete.bind(this);
  }
}

Here is a fixed version of the NotesListItem class.

class NotesListItem extends Component {
  constructor(props) {
    super(props);

    this.delete = this.delete.bind(this);
  }

  delete() {
    this.props.deleteNote(this.props.note);
  }

  render() {
    return (
      <li className="notes-list-item">
        <span className="item-text">{this.props.note.text}</span> {/* <-- using props */}
        <div className="notes-btn-group btn-group" role="group">
          <button
            className="delete-btn btn btn-danger"
            type="button"
            onClick={this.delete}
          >
            &times;
          </button>
        </div>
      </li>
    );
  }
}