-
-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
assoc-in #11
Comments
Bringing this discussion from #17 here @borkdude @corasaurus-hex @borkdude wrote:
I think we should have both For export default function appReducer(state = initialState, action) {
switch (action.type) {
case 'todos/todoAdded': {
return {
...state,
todos: [
...state.todos,
{
id: nextTodoId(state.todos),
text: action.payload,
completed: false
}
]
}
}
case 'todos/todoToggled': {
return {
...state,
todos: state.todos.map(todo => {
if (todo.id !== action.payload) {
return todo
}
return {
...todo,
completed: !todo.completed
}
})
}
}
case 'filters/statusFilterChanged': {
return {
// Copy the whole state
...state,
// Overwrite the filters value
filters: {
// copy the other filter fields
...state.filters,
// And replace the status field with the new value
status: action.payload
}
}
}
default:
return state
}
} This can be written using immutable (defn app-reducer [state action]
(case (:type action)
:todos/todo-added
(update state :todos conj {:id (next-todo-id state)
:text (:payload action)
:completed false})
:todos/todo-toggles
(assoc state :todos
(map #(if (= (:id %) (:payload action))
(update % :completed not)
%)
(:todos state)))
:todos/status-filter-change
(assoc-in state [:filters :status] (:payload action)))) |
Agreed: assoc-in should be fully "shallow" immutable and assoc-in! can be in-place updating. This would seem the least surprising behavior. PRs welcome for each. |
I guess I should split up my open PR then 😅 |
@corasaurus-hex If you could change your current implementations to align with the above, for now I'm fine with merging your current PR and then in the future, let's work in smaller chunks. |
luckily they already work this way so I don't need to make any changes 😄 |
maybe I should change the tests for assoc-in to demonstrate that they're not the same objects, like how for assoc-in! I demonstrate that they are the same objects? |
Good idea! |
I merged your PR, feel free to provide incremental PRs with tests. |
No description provided.
The text was updated successfully, but these errors were encountered: