Fix bug caused by inconsistent Todo state across components in the Basic chapter of the Docs #795
Conversation
|
Woah, that's a really good catch. However I'd rather opt for generating unique IDs when creating todos. What do you think? |
|
Yes, I got the id-driven approach to work by tweaking the todo reducer and the createTodo action in this branch, and this sort of approach is much more likely to pay off as the app scales. While the functional solution above is an inefficient solution by way of requiring all the todos to be present in the props at all the time, it could be molded into a rigid exercise of keeping the state constant throughout the app and not letting it mutate outside of reducers. At the end, there could be a sidebar where it is converted over to using ids, showing how they allow us to slim down the state drastically from where the purely functional approach could go. In the end, I'm uncertain which way would be best to use as a tutorial for someone who is unfamiliar with React + Flux-ish things. On one hand, the approach of the tutorial right now emphasizes the functional basis of Redux in a way that plucking items out of a tree by their ID doesn't. As the examples grow more complex, introducing Ids is inevitable; however, for the first exercise, it may be a bit overkill, and it robs the beginner of the lesson. All in all, it's a choice of whether to introduce the concept of IDs or reinforce the idea of keeping the structure of the state consistent to the point where the structure itself controls how the app operates. |
|
Closing as this particular PR is outdated. |
|
This is fixed in all examples now. |
The tutorial for the TodoMVC app in documentation, the visibleTodos prop is passed to TodoList, which is mapped into a new array -- which I will call visibleTodos -- and rendered into Todo components. Upon doing so, the visibleTodos array now is at risk of becoming unlinked with the state.todos array as soon as a single item is completed.
Because the completeTodo reducer relies on parity between the indexes of the visibleTodos array and the state.todos array, bugs can occur when in trying to dispatch a completeTodo action in SHOW_ACTIVE view mode. The bug occurs because the visibleTodos array can be shorter than the state.todos array, and any index in the list can be filtered out once completed.
In the screen shot below, "third" has already been completed. When clicking on "fourth" todo -- the fourth item in the state.todos array and third item in the visibleTodos array, the payload informs the app that it should complete the third item in the array, so nothing happens.
Likewise, when clicking on the "fifth" todo --- fifth in state.todos but fourth in visibleTodos, it completes the "fourth" item by dispatching a payload targeting the index: 3.
I have crafted two solutions. The first is to map a uuid to each todo and have the completeTodo action splice the array using the index returned by Array.prototype.findIndex(todo.id) instead of the current position of the Todo in the visibleTodos array. This solution fixes the inaccuracies of the reducer, but it also requires actions and the reducer to be rewritten and another module to be installed. I'm not a fan.
Instead, why not pass the filtering mechanisms themselves to TodoList component via props by altering the selectTodos helper to return functions instead of filtered arrays? Now we are free to pass the entire state.todos array to TodoList and simply not render any todo within it that doesn't return true when passed to the filtering function. This keeps our state consistent across components without altering any reducers or actions. I believe this to be the elegant solution. It is included in this PR.
My apologies for such a big switch.