-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix: delete root keys with immer middleware #2481
base: main
Are you sure you want to change the base?
Conversation
* Fix bug described in pmndrs#2480 * Add basic tests for the immer middleware
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review all comments
@simlmx we need to add a comment on immer middleware saying that |
Is this accurate? I thought the vanilla version was still intended to be supported with
This behavior makes sense to me in the context of a middleware: intercepting only the 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. |
@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 |
@connormanning let's discuss the changes to immer middleware here |
oookay, I guess we have some design conflicts. 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 |
Or, maybe: setState(
nextStateOrUpdater: T | ((state: Draft<T>) => T | void),
...a: SkipTwo<A>
): Sr |
@dai-shi yeah, I guess we need to implement this on v5 |
Related Issues or Discussions
Fixes #2480
Summary
Currently when calling
setState
with theimmer
middleware,replace=false
is assumed by default. This means that the old state is merged with immer'sproduce
output. I don't think there is any reason to merge those states: immer'sproduce
already returns a smart version of the whole state where copies are used whenever possible. The merging is mostly equivalent toreplace=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 likeand never
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
yarn run prettier
for formatting code and docs@dai-shi @dbritto-dev Let me know if I can improve anything in the PR.