Skip to content
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

[Refactor] Update all React Toolkit slices to use mutating syntax #358

Closed
taniarascia opened this issue Oct 6, 2020 · 3 comments · Fixed by #361
Closed

[Refactor] Update all React Toolkit slices to use mutating syntax #358

taniarascia opened this issue Oct 6, 2020 · 3 comments · Fixed by #361
Labels
Status: Completed Type: Refactor Code change without bugfix or added feature

Comments

@taniarascia
Copy link
Owner

Use:

state.x = true

Instead of:

{
  ...state,
  x: true,
}
@taniarascia taniarascia added Type: Feature New feature or request Type: Refactor Code change without bugfix or added feature Good first issue Good for newcomers and removed Type: Feature New feature or request labels Oct 6, 2020
@taniarascia taniarascia linked a pull request Oct 7, 2020 that will close this issue
3 tasks
@taniarascia taniarascia removed the Good first issue Good for newcomers label Oct 7, 2020
taniarascia added a commit that referenced this issue Oct 7, 2020
* Fix settings

* Refactor

* Update

* Templates
@markerikson
Copy link

Note that additional "mutation" could be done here. For example, in slices/notes.ts, there's a number of cases like this:

    toggleFavoriteNotes: (state, { payload }: PayloadAction<string>) => {
      state.notes = state.selectedNotesIds.includes(payload)
        ? state.notes.map((note) =>
            state.selectedNotesIds.includes(note.id) ? { ...note, favorite: !note.favorite } : note
          )
        : state.notes.map((note) =>
            note.id === payload ? { ...note, favorite: !note.favorite } : note
          )
    },

That could potentially be rewritten along these lines:

    toggleFavoriteNotes: (state, { payload }: PayloadAction<string>) => {
      const toggleSelected = state.selectedNotesIds.includes(payload);
      
      // Can use a `forEach` loop instead of a `map()` - no need to create a new array manually
      state.notes.forEach(note => {
        const shouldToggle = toggleSelected ? state.selectedNotesIds.includes(note.id) : note.id === payload
        if (shouldToggle) {
          // Can directly "mutate" a given note entry
          note.favorite = !note.favorite
        }
      })
    },

@taniarascia
Copy link
Owner Author

@markerikson Thanks for the tip! I went through and only updated the "top level" mutations, but I definitely left a lot of map and filter where it could be refactored to forEach. That looks way cleaner.

@markerikson
Copy link

Yeah. One of the great things about Immer is that you can grab out nested values, "mutate" those nested values seemingly by themselves, and Immer does all the work of tracking the parents that need to get copied too. Like, in this case, note.favorite = !note.favorite triggers copies of notes and state automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Completed Type: Refactor Code change without bugfix or added feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants