Pushing to array in React, unexpected behaviour

Question

Trying to make a todo-list app

I want my todo items to have a unique key/id. I want to create the todo item id by taking the last todo item and adding 1 to its id. If the item does not exist it creates a new one with id = 1.

App.js:

import React, {Component} from 'react';
import Todos from './Todos'
import AddTodo from './AddForm'

class App extends Component {

  state = {
    todos: [
      {id: 1, content: 'buy milk'},
      {id: 2, content: 'play nintendo'}
    ]
  }

 addTodo = (todo) => {
    if (this.state.todos[0])
    {
      todo.id = this.state.todos[this.state.todos.length-1].id +1;
      let todos = [...this.state.todos];
      todos.push(todo);
      this.setState({
        todos: todos
      });
    }
    else
    {
      this.setState({
        todos: [
          {id: 1, content: todo.content}
        ]
      });
    }
    console.log(this.state.todos);
  }


  render() {
    return(
      <div className="todo-app container">
        <h1 className="center blue-text">Todos:</h1>
        <Todos deleteTodo={this.deleteTodo} todos={this.state.todos}/>
        <AddTodo addTodo={this.addTodo} />
      </div>
    );
  }
}

This almost works. I can add new items if the items have a unique content, but problems occur when the items have the same content.

Unique content console.log:

0: {content: "buy milk", id: 1}
1: {content: "play nintendo", id: 2}
2: {content: "adrfhahhafafdhafhafhhfa", id: 3}
3: {content: "adrfhahhafafdhafhafhhfaafhafhfah", id: 4}
4: {content: "adrfhahhafafdhafhafhhfaafhafhfahafhfahafh", id: 5}
5: {content: "adrfhahhafafdhafhafhhfaafhafhfahafhfahafhafhafhfah", id: 6}

Same content console.log:

0: {content: "buy milk", id: 1}
1: {content: "play nintendo", id: 2}
2: {content: "adrfhahhafafdhafhafhhfa", id: 3}
3: {content: "adrfhahhafafdhafhafhhfaafhafhfah", id: 4}
4: {content: "adrfhahhafafdhafhafhhfaafhafhfahafhfahafh", id: 5}
5: {content: "adrfhahhafafdhafhafhhfaafhafhfahafhfahafhafhafhfah", id: 7}
6: {content: "adrfhahhafafdhafhafhhfaafhafhfahafhfahafhafhafhfah", id: 7}

As you can see the id is the same if the content is the same.

I know that this is probably the wrong way to add id:s in react, but I want to understand what is going on. Thanks in advance.

EDIT: Yevgen pointed out a flaw with my logic if items are deleted, but I am still have not understood why the ID:s are repeating. Someone asked for the rest of the code so I added it all.

AddForm.js

import React, {Component} from 'react';

class AddTodo extends Component {
  state = {
    content: ''
  }

  handleChange = (e) => {
    this.setState({
      content: e.target.value
    });
  }

  handleSubmit = (e) => {
    e.preventDefault();
    this.props.addTodo(this.state);
  }

  render(){
    return (
    <div>
      <form onSubmit={this.handleSubmit}>
        <label>Add new todo</label>
        <input type="text" onChange={this.handleChange} />
      </form>
    </div>
    );
  }
}

export default AddTodo;

Todos.js

import React from 'react'

const Todos = ({todos, deleteTodo}) => {
  const todoList = todos.length ? (
    todos.map(todo => {
      return (
        <div className="collection-item" key={todo.id}>
          <span onClick={() => {deleteTodo(todo.id)}}>{todo.content}</span>
        </div>
      )
    })
  ) : (
    <p className="center">You have no todos left</p>
  )
  return (
    <div className="todos collection">
      {todoList}
    </div>
  )
}

export default Todos;

The issue seems to be in the todo passed when calling addTodo. As the array of todos nor a single todo is being passed to the AddTodo component, I wonder what are you passing to it when calling addTodo(?). Could you share that part of the code? Thanks — Alvaro
– Alvaro, Commented Feb 29, 2020 at 20:27
@AmerllicA I know it doesn't follow the usual paradigm, but if you're sure the object you're mutating is new and not in state, what harm does it cause? Sometimes, the logic of code with mutations is clearer than code without mutations (though this isn't a good example of that, since it's easy to use rest syntax here) — CertainPerformance
– CertainPerformance, Commented Feb 29, 2020 at 20:58
@Alvaro I added the rest of the code. I still dont understand why the ID:s are repeating if the content is the same... — Asterix Assa
– Asterix Assa, Commented Mar 1, 2020 at 13:06
@AsterixAssa : please, consider suggestions from my updated answer. Hopefully, now I got all of your confusions addressed thoroughly. Hope, it finally gets me an accept ;) — Yevhen Horbunkov
– Yevhen Horbunkov, Commented Mar 1, 2020 at 21:01

Yevhen Horbunkov · Accepted Answer · 2020-03-01 20:59:00Z

3

My guess is that your issue comes from the way you declare <AddForm /> component's state. You should have done that within constrictor body, as this.state. What you do currently is global variable declaration. So, it isn't bound to your app state and your id is not getting assigned properly.

Other than that, your addTodo() looks way too overcomplicated and error prone (you can't rely on array length to get maximum used id, since deleting items, other than last will break the logic).

Besides, it's usually recommended to avoid using arrow notation for class methods. Instead, you should declare those as regular functions and do something, like this.addTodo = this.addTodo.bind(this) within constructor body for each method you declare. Otherwise, if you like arrow functions that much, you may enjoy those even more with function components.

Following your logic to assign autoincremented id's (thus, id's of deleted items never get reused) and passing object with property content to addTodo(), you could've simply done:

addTodo({content}){
   const id = Math.max(...this.state.todos.map(({id}) => id),0)+1
   this.setState({todos:[...this.state.todos, {id, content}]})
}

edited Mar 1, 2020 at 20:59

answered Feb 29, 2020 at 20:38

Yevhen Horbunkov

15.6k3 gold badges27 silver badges46 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

CertainPerformance Over a year ago

While this looks like an improvement, I'm not sure it answers the question of why the IDs are getting repeated - but there doesn't look to be enough information in the question for a real answer

Asterix Assa Over a year ago

Thank you for this. I realise the error I made with the array logic when deleting items. I am still confused though. The reason I am using arrow functions is so that I can access the classes "this.state.". If I use your function in my class it gives me an error: "this.state.todos not defined"... Any advice?

Asterix Assa · Accepted Answer · 2020-03-01 13:51:27Z

I was able to get it working by fixing this:

 if (this.state.todos[0])
    {
      todo.id = this.state.todos[this.state.todos.length-1].id +1;
      let todos = [...this.state.todos];
      todos.push(todo);
      this.setState({
        todos: todos
      });
    }

With this:

 if (Array.isArray(this.state.todos) && this.state.todos.length)
    {
      todo.id = Math.max(...this.state.todos.map(({id}) => id),0)+1
      let todos = [...this.state.todos];
      todos.push({id: todo.id, content: todo.content})
      this.setState({
        todos: todos
      })
    }

Yevgen pointed out my issue with id names when deleting todo-list items. I fixed it. Thank you.

The repeating id names was somehow tied to the way i was pushing to the array... I still dont understand it fully, but it works now..

Collectives™ on Stack Overflow

Pushing to array in React, unexpected behaviour

2 Answers 2

2 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related