State is not updated properly when using React effects

Question

I'm working on a small todo app as an exercise using React. I have a mock service like this:

export default class TodoService {

    constructor(todos) {
        this.todos = new Map();
        todos.forEach(todo => {
            this.todos.set(todo.id, todo);
        });
    }

    findAll() {
        return Array.from(this.todos.values());
    }

    saveTodo(todo) {
        this.todos[todo.id] = todo
    }

    completeTodo(todo) {
        this.todos.delete(todo.id)
    }
}

and in my React app I have some state which contains the todos:

const [todos, setTodos] = useState([]);
const [flip, flipper] = useState(true);

const completeTodo = (todo) => {
    todoService.completeTodo(todo);
    flipper(!flip);
}

useEffect(() => {
    setTodos(todoService.findAll());
}, [flip])

completeTodo is a function which I pass into my Todo component to be used when I want to complete a todo like this:

import React from "react";
const Todo = ({ todo, completeFn }) => {
    return (
        <form className="todo">
            <div className="form-check">
                <input
                    className="form-check-input"
                    type="checkbox"
                    value=""
                    name={todo.id}
                    id={todo.id}
                    onClick={() => {
                        console.log(`completing todo...`)
                        completeFn(todo)
                    }} />
                <label className="form-check-label" htmlFor={todo.id}>
                    {todo.description}
                </label>
            </div>
        </form>
    )
}
export default Todo

So what happens is that whenever the user clicks the checkbox completeFn is called with the todo, it gets deleted from the service object and the state is supposed to update, but the weirdest thing happens.

When TodoService.completeTodo() is called the todo gets deleted properly, but when findAll() is called the old todo is still there! If I write the contents to the console I can see the item being deleted then somehow teleported back when I call findAll. Why does this happen? I this because of some React magic I don't understand?

Edit: What's even more insane is that if I modify this to only use effects for the initial loading like this:

const [todos, setTodos] = useState([]);

const completeTodo = (todo) => {
    todoService.completeTodo(todo);
    setTodos(todoService.findAll());
}

useEffect(() => {
    setTodos(todoService.findAll());
}, [])

I get a super weird result:

Can someone explain this to me?

Edit2: This is a complete reproducible example (without the index.html with a <div id="root"></div> in it).

import React, { useState, useEffect } from "react";
import ReactDOM from "react-dom";

const Todo = ({ todo, completeFn }) => {
    return (
        <div>
            <input
                type="checkbox"
                name={todo.id}
                id={todo.id}
                onClick={() => {
                    console.log(`completing todo...`)
                    completeFn(todo)
                }} />
            <label className="form-check-label" htmlFor={todo.id}>
                {todo.description}
            </label>
        </div>
    )
}

class TodoService {

    constructor(todos) {
        this.todos = new Map();
        todos.forEach(todo => {
            this.todos.set(todo.id, todo);
        });
    }

    findAll() {
        return Array.from(this.todos.values());
    }

    saveTodo(todo) {
        this.todos[todo.id] = todo
    }

    completeTodo(todo) {
        this.todos.delete(todo.id)
    }
}

const App = () => {

    let todoService = new TodoService([{
        id: 1,
        description: "Let's go home."
    }, {
        id: 2,
        description: "Take down the trash"
    }, {
        id: 3,
        description: "Play games"
    }]);

    const [todos, setTodos] = useState([]);
    const [flip, flipper] = useState(true);

    const completeTodo = (todo) => {
        todoService.completeTodo(todo);
        flipper(!flip);
    }

    useEffect(() => {
        setTodos(todoService.findAll());
    }, [flip])

    return (
        <div>
            {todos.map(todo => <Todo key={todo.id} todo={todo} completeFn={completeTodo} />)}
        </div>
    )
};
ReactDOM.render(<App />, document.getElementById("root"));

Is it possible for you to create a minimal reproducible example — Atin Singh, May 07 '20 at 21:31
I think the issue here is basically that you're trying to create a state object which exists outside the React render loop. When `flipper` gets called, the component is rerendered, and a new instance of `todoService` is created. The `todoService` you just updated belongs to the previous render cycle. You need to find some way of combining the functionality of the `todo` state, and the `todoService`, which is what custom hooks are for. — lawrence-witt, May 07 '20 at 22:00
I just realized that I create the mock todos within the `App`. I'm so stupid. But is there a better way of forcing the state update than the flip flop thing I do? — Adam Arold, May 07 '20 at 22:05
you're also setting state inside the `useEffect` https://stackoverflow.com/questions/53715465/can-i-set-state-inside-a-useeffect-hook — MonteCristo, May 07 '20 at 22:11

MonteCristo · Answer 1 · 2020-05-07T22:40:24.810

You don't need to call useEffectin this scenario. You've put a dependency in the useEffect which is fine to use it to stop infinite loop. but it's unnecessary here. You're not really doing any fetch

You can update your code to be like this.

import React, { useState, useCallback, useEffect } from "react";

const Todo = ({ todo, completeFn }) => {
  const handleOnclick = useCallback(() => {
    // useCallback since function is passed down from parent
    console.log(`completing todo...`);
    completeFn(todo);
  }, [completeFn, todo]);

  return (
    <div>
      <input
        type="checkbox"
        name={todo.id}
        id={todo.id}
        onClick={handleOnclick}
      />
      <label className="form-check-label" htmlFor={todo.id}>
        {todo.description}
      </label>
    </div>
  );
};

class TodoService {
  constructor(todos) {
    this.todos = new Map();
    todos.forEach(todo => {
      this.todos.set(todo.id, todo);
    });
  }

  findAll() {
    console.log(Array.from(this.todos.values()));
    return Array.from(this.todos.values());
  }

  saveTodo(todo) {
    this.todos[todo.id] = todo;
  }

  completeTodo(todo) {
    this.todos.delete(todo.id);
  }
}

const todoService = new TodoService([
  {
    id: 1,
    description: "Let's go home."
  },
  {
    id: 2,
    description: "Take down the trash"
  },
  {
    id: 3,
    description: "Play games"
  }
]);

export default function App() {
  const [todos, setTodos] = useState([]); // Set initial state

  const completeTodo = todo => {
    todoService.completeTodo(todo);
    setTodos(todoService.findAll()); // Update state
  };

  useEffect(() => {
    setTodos(todoService.findAll());
  }, []); // Get and update from service on first render only

  return (
    <div>
      {todos.map(todo => (
        <Todo key={todo.id} todo={todo} completeFn={completeTodo} />
      ))}
    </div>
  );
}

working example

https://codesandbox.io/s/cranky-hertz-sewc5?file=/src/App.js

@AdamArold I see ok... then that should be fine you can just use `[]` as your `useEffect` dependency. This will ensure that `useEffect` will run in initial render only. After that you'd want to update the state when user select complete. As shown above. that's probably the important part — MonteCristo, May 07 '20 at 22:30
I need to use `useEffect` because `todoService.findAll()` will call an API later. — Adam Arold, May 07 '20 at 22:32
updated the answer. check https://codesandbox.io/s/cranky-hertz-sewc5. Also note I've used `useCallback` for `completeFn` since it's passed down from parent. — MonteCristo, May 07 '20 at 22:41

State is not updated properly when using React effects

1 Answers1