fix: delete root keys with immer middleware

[simlmx]

Related Issues or Discussions

Fixes #2480

Summary

Currently when calling setState with the immer middleware, replace=false is assumed by default. This means that the old state is merged with immer's produce output. I don't think there is any reason to merge those states: immer's produce already returns a smart version of the whole state where copies are used whenever possible. The merging is mostly equivalent to replace=True, except when keys are deleted: the old deleted keys get merged with the new state.

I'm also assuming that when using the immer middleware, we always want to do something like

store.setState((state) => { state.x = 3})

and never

store.setState({x: 3})

so I simplified the middleware consequently.

I added a test to reproduce the bug and some other simple tests.

Someone with more experience with the immer middleware should double-check all this! There could very well be usage patterns that I'm not aware of.

Check List

• [x] yarn run prettier for formatting code and docs

@dai-shi @dbritto-dev Let me know if I can improve anything in the PR.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
zustand-demo	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 12, 2024 4:07am

[codesandbox-ci[bot]]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[dbritto-dev]

@simlmx we need to add a comment on immer middleware saying that shouldReplace parameter is completely ignore on immer middleware and will be set up to true by default

[connormanning]

I'm also assuming that when using the immer middleware, we always want to do something like

store.setState((state) => { state.x = 3})

and never

store.setState({x: 3})

Is this accurate? I thought the vanilla version was still intended to be supported with immer if you do return a value from your updater or supply a partial state:

• returning nothing applies any in-place Draft modifications
• returning a value applies the vanilla shallow merge behavior
• doing both is an error: caught Error: [Immer] An immer producer returned a new value *and* modified its draft. Either return a new value *or* modify the draft

This behavior makes sense to me in the context of a middleware: intercepting only the void returns. For example you could use the immer model only for a few operations which operate on nested states, or another example being adding immer to an existing store for a few operations without having to rewrite all of the existing vanilla operations.

I'm no expert on this project so I'll defer to the maintainers, but I think #2495, which explicitly adds TS definitions to support the model described above, contradicts the assumptions in this PR. Hopefully someone with a greater understanding of this library and its middleware can chime in.

[dbritto-dev]

@connormanning when you use immer you return the whole state so you should merge the current state with the new state you should always replace the state since immer handle the changes, this PR set replace true by default

[dbritto-dev]

@connormanning let's discuss the changes to immer middleware here

[dai-shi]

oookay, I guess we have some design conflicts. I guess the question if whether we should force replace=true with immer middleware.

As I generally prefer simpler api and implementation, my suggestion is:

        setState(
          nextStateOrUpdater: (state: Draft<T>) => T | void,
          ...a: SkipTwo<A>
        ): Sr

But, this is a breaking change, so it should only come in v5.

And, my suggestion is, if this does't cover some use cases, use produce without the middleware. (we need to educate it in docs.)

[dai-shi]

Or, maybe:

        setState(
          nextStateOrUpdater: T | ((state: Draft<T>) => T | void),
          ...a: SkipTwo<A>
        ): Sr

[dbritto-dev]

@dai-shi yeah, I guess we need to implement this on v5

[dbritto-dev]

Let's create a new PR if is necessary I'll close this one for now