Fix Immer type inference for `setState` #2696

chrisvander · 2024-08-23T02:57:05Z

Bases the type of setState off of setState instead of getState.

Related Bug Reports or Discussions

Summary

This PR fixes the Immer middleware so that the type of setState is inferred off of the store's setState as opposed to getState. When using a middleware like zustand-computed, this would previously have the result of a differing getState generic would move over to setState unintentionally.

Check List

pnpm run prettier for formatting code and docs

… `setState` instead of `getState`

vercel · 2024-08-23T02:57:09Z

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	Aug 27, 2024 1:09am

codesandbox-ci · 2024-08-23T02:57:25Z

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.

dai-shi · 2024-08-23T06:59:41Z

src/middleware/immer.ts

-          shouldReplace: true,
-          ...a: SkipTwo<A2>
-        ): Sr2
+  ? SetState extends StoreApi<infer T>['setState']


I'm not sure if this works because StoreApi['setState'] is very complicated type. Also, setState can be modified by upstream middleware. Can we infer T from A1 and A2?

It does infer correctly as the StoreApi setState type is based off of the single generic passed into StoreApi. I think doing it this way bottles the complexity of the type and handles it appropriately. All the tests passed and it seems to resolve correctly.

When I tried to base it off of A1 and A2, I would've needed to fight through the entire intersection type (e.g. T | Partial<T> | ...) in order to get T. That method feels more brittle than inferring through StoreApi.

Any situations you're thinking of specifically here? Like a modified setState type from another middleware?

Well, actually, if I used the function type, there would be no Partial to fight with.

Like a modified setState type from another middleware?

Yes.

Check out my latest commit and let me know what you think of that. The assumption I made is that A2 will always have a first parameter that is T | (...args: any[]) => any. So, T can be found by excluding any function types. That should be more robust.

Inferring off of A1 seemed non-trivial given that it is T | Partial<T> | ..., which I'm not sure if there's a method to derive T out of that without making further assumptions about the structure of setState.

Hmmm, interesting. I guess the approach is right though.

…tional component of state

dai-shi

this is too tricky and not 100% correct anyway. seems fine for now.
can you add a test please?

src/middleware/immer.ts

chrisvander · 2024-08-24T22:45:29Z

this is too tricky and not 100% correct anyway. seems fine for now. can you add a test please?

The simplest test would probably be to register an example middleware that modifies getState but not setState and see if it resolves correctly downstream. Does that sound reasonable?

chrisvander · 2024-08-24T22:57:05Z

Check out that new test and let me know if it's sufficient. Pollution of the StoreMutators type should be isolated to the test file.

dai-shi · 2024-08-25T02:03:23Z

Thanks for working on it.

Pollution of the StoreMutators type should be isolated to the test file.

Hmm, but it made me think that it should be safer if the new test is put in a new test file. What do you think?

chrisvander · 2024-08-25T11:38:51Z

Thanks for working on it.

Pollution of the StoreMutators type should be isolated to the test file.

Hmm, but it made me think that it should be safer if the new test is put in a new test file. What do you think?

I'd be okay with that, but it does make sense alongside other middleware tests. For most of the middleware it may be nice to have some contrived third party middleware examples to try things against. Outside the scope of this PR but thought I would mention.

dai-shi · 2024-08-25T12:16:06Z

tests/middlewareTypes.test.tsx

+type StoreModifyAllButSetState<S, A> = S extends {
+  getState: () => infer T
+}
+  ? Omit<StoreApi<T & A>, 'setState'>


I'm not sure if I understand this fully, but omitting setState may prevent inferring type from setState, no?
The reason the immer impl works is it's using set instead of setState, but we don't get types from set.
I'm a bit confused what's going on.

Sorry, just updated that. The goal is to use an example middleware that modifies the store state to add some A type to all but the setState, and then assert that it affected the types appropriately on both the inside of the Immer middleware and the outside.

yeah, but still it doesn't feel like a good example, because the implementation of the example middleware should include delete api.setState, but immer middleware infers types from setState.

Why would the implementation need to include delete api.setState? The example rewrites setState, it doesn't delete it.

Do you agree that the type assertions are sufficient? Since this change is strictly on a type level, I didn't think implementation was necessary.

it doesn't delete it.

Okay, I think I misread something then.

yeah, I think I forgot that Write is an intersection.
Never mind.

dai-shi

LGTM
Thanks for your contribution!

(Now, I wonder if we should cut an rc release only for this change. 🤔 )

chrisvander · 2024-08-26T02:20:38Z

A minor bump may not hurt 😁

fix(immer): tweak type inference to base setState type off of store…

7d4267f

… `setState` instead of `getState`

chrisvander changed the title ~~fix(immer): tweak type inference to base setState type off of store…~~ Fix Immer type inference for setState Aug 23, 2024

vercel bot deployed to Preview August 23, 2024 02:57 View deployment

chrisvander marked this pull request as draft August 23, 2024 02:58

fix(immer): instead, infer type directly from StoreApi<T>["setState"]

e887294

vercel bot deployed to Preview August 23, 2024 03:09 View deployment

chrisvander marked this pull request as ready for review August 23, 2024 03:09

This was referenced Aug 23, 2024

I'm using a combination of immer middleware and typescript, but my computedState type doesn't exist! chrisvander/zustand-computed#24

Closed

Typing of get() does not include computed state chrisvander/zustand-computed#15

Closed

dai-shi reviewed Aug 23, 2024

View reviewed changes

fix(immer): instead of using StoreApi, extract from A2 the non-func…

f12c3ae

…tional component of state

vercel bot deployed to Preview August 23, 2024 13:14 View deployment

dai-shi requested changes Aug 24, 2024

View reviewed changes

src/middleware/immer.ts Show resolved Hide resolved

docs: add comment describing why it is not derived from A1

1e86925

vercel bot deployed to Preview August 24, 2024 22:12 View deployment

test: add example middleware that modifies getState w/o setState

3c5ac54

vercel bot deployed to Preview August 24, 2024 22:55 View deployment

dai-shi reviewed Aug 25, 2024

View reviewed changes

vercel bot deployed to Preview August 25, 2024 12:53 View deployment

fix: add assertion for inner set and get types

1067033

chrisvander force-pushed the base-immer-middleware-off-setstate branch from 1540bfd to 1067033 Compare August 25, 2024 13:00

vercel bot deployed to Preview August 25, 2024 13:01 View deployment

dai-shi approved these changes Aug 26, 2024

View reviewed changes

Merge branch 'main' into base-immer-middleware-off-setstate

b7914a3

vercel bot deployed to Preview August 27, 2024 01:09 View deployment

dai-shi merged commit d7345da into pmndrs:main Aug 27, 2024
25 checks passed

chrisvander deleted the base-immer-middleware-off-setstate branch August 27, 2024 02:59

Evan-2007 mentioned this pull request Nov 8, 2024

[Snyk] Upgrade zustand from 5.0.0-rc.2 to 5.0.0 Evan-2007/Amplitune#5

Open

snyk-io bot mentioned this pull request Nov 9, 2024

[Snyk] Upgrade zustand from 4.1.5 to 5.0.0 doperiddle/chainlist#8

Open

welllucky mentioned this pull request Nov 9, 2024

[Snyk] Upgrade zustand from 5.0.0-rc.2 to 5.0.0 welllucky/Services#79

Open

snyk-io bot mentioned this pull request Nov 10, 2024

OPHJOD-851: Upgrade zustand from 5.0.0-rc.2 to 5.0.0 Opetushallitus/jod-yksilo-ui#260

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Immer type inference for `setState` #2696

Fix Immer type inference for `setState` #2696

chrisvander commented Aug 23, 2024 •

edited

Loading

vercel bot commented Aug 23, 2024 •

edited

Loading

codesandbox-ci bot commented Aug 23, 2024 •

edited

Loading

dai-shi Aug 23, 2024

chrisvander Aug 23, 2024

chrisvander Aug 23, 2024 •

edited

Loading

dai-shi Aug 23, 2024

chrisvander Aug 23, 2024

chrisvander Aug 23, 2024 •

edited

Loading

dai-shi Aug 24, 2024

dai-shi left a comment

chrisvander commented Aug 24, 2024

chrisvander commented Aug 24, 2024

dai-shi commented Aug 25, 2024

chrisvander commented Aug 25, 2024

dai-shi Aug 25, 2024

chrisvander Aug 25, 2024

dai-shi Aug 25, 2024

chrisvander Aug 26, 2024

dai-shi Aug 26, 2024

dai-shi Aug 26, 2024

dai-shi left a comment

chrisvander commented Aug 26, 2024

Fix Immer type inference for setState #2696

Fix Immer type inference for setState #2696

Conversation

chrisvander commented Aug 23, 2024 • edited Loading

Related Bug Reports or Discussions

Summary

Check List

vercel bot commented Aug 23, 2024 • edited Loading

codesandbox-ci bot commented Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisvander Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisvander Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dai-shi left a comment

Choose a reason for hiding this comment

chrisvander commented Aug 24, 2024

chrisvander commented Aug 24, 2024

dai-shi commented Aug 25, 2024

chrisvander commented Aug 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dai-shi left a comment

Choose a reason for hiding this comment

chrisvander commented Aug 26, 2024

Fix Immer type inference for `setState` #2696

Fix Immer type inference for `setState` #2696

chrisvander commented Aug 23, 2024 •

edited

Loading

vercel bot commented Aug 23, 2024 •

edited

Loading

codesandbox-ci bot commented Aug 23, 2024 •

edited

Loading

chrisvander Aug 23, 2024 •

edited

Loading

chrisvander Aug 23, 2024 •

edited

Loading