fix(react, types): relax react types

[lukesmurray]

Summary

UseBoundStore strictly enforces the shape of the setState function but does not rely on the shape of the setState function internally.

Why we should be relaxed about setState

I want to define a middleware that makes the action in devtools required (to improve debugging store mutations in a large app). But if I make the third argument to setState required, the modified setState function is no longer compatible with the default setState function.

We get the following error message (edited for readability).

Types of property 'setState' are incompatible.

'<A extends string | { type: unknown; }>(
   partial: T | Partial<T> | ((state: T) => T | Partial<T>), 
   replace: boolean | undefined, 
   action: A
) => void' 

is not assignable to 

'(partial: unknown, replace?: boolean | undefined) => void'.

Internally the react extension only seems to rely on getState and subscribe, so I made that explicit in the types.

Check List

• yarn run prettier for formatting code and docs

[codesandbox-ci]

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.

Latest deployment of this branch, based on commit 74fed9f:

Sandbox	Source
React	Configuration
React Typescript	Configuration
React Browserify	Configuration
React Snowpack	Configuration
React Parcel	Configuration
@pavlobu/zustand demo	Configuration

[dai-shi]

I don't think this is correct, or helps anyways. We do use setState on useBoundStore.
Have you made your vanilla store type correctly?
Are you already familiar with typing simpler middleware?

Closing. You can continue discussion here or open a new discussion.

[lukesmurray]

Thank you for the prompt response and for creating and maintaining zustand!

• The useStore code doesn't seem to access setState. Is there something I'm missing? If useBoundStore uses setState then this change is definitely wrong and the issue should be closed.

zustand/src/react.ts

Lines 33 to 47 in ab4118d

	export function useStore<TState, StateSlice>(
	api: WithReact<StoreApi<TState>>,
	selector: (state: TState) => StateSlice = api.getState as any,
	equalityFn?: (a: StateSlice, b: StateSlice) => boolean
	) {
	const slice = useSyncExternalStoreWithSelector(
	api.subscribe,
	api.getState,
	api.getServerState || api.getState,
	selector,
	equalityFn
	)
	useDebugValue(slice)
	return slice
	}

• I'm new to typing middleware but I'm pretty sure the middleware is correctly typed. Here is an example with the error. If we can relax setState using the changes I proposed the error would go away since the middleware would be allowed to overwrite setState to be a function that is incompatible with the vanilla setState call.

Here is an image of the error in case it doesn't show up for you but I assume it will.

[dai-shi]

If useBoundStore uses setState then this change is definitely wrong and the issue should be closed.

Users can use it. See: https://tsplay.dev/wQYPvw

I'm new to typing middleware but I'm pretty sure the middleware is correctly typed. Here is an example with the error.

Hmm, interesting.
We may need to ask @devanshj for help.
Here's the same one as stackblitz: https://tsplay.dev/N5O1MW
(We may need to make the type bivariant??)

[devanshj]

I'm pretty sure the middleware is correctly typed

Your WithDevtoolsReq<StoreApi<T>> is not a subtype of StoreApi<T>, so there's a very good chance the middleware typings are incorrect. Could you please share a minimal repro on TypeScript playground with your middleware typings. EDIT: oops my bad you have already linked a stackblitz haha, afk let me check that out in sometime.

Technically we could not ask for the setState as we don't use it in the implementation (the users would still be able to use setState) but in general it's not a good practice to fine-grain types to such an extent. In the worst case we could do that but that's not needed here.

[devanshj]

To save me from manually diffing could you tell me in what way is your devtoolsReq different from devtools? Because the code compiles using devtools instead of devtoolsReq.

[lukesmurray]

only difference is the action is required instead of optional.

devtools has
(...skipTwo, action?: A)

devtoolsReq has
(...skipTwo, action: A)

[devanshj]

Yeah I too just got that diff. So assuming it's an intended change, we'd need fine-grained type to allow passing a store with such middleware.

In that case we can reopen this PR.

[devanshj]

Instead of this let's do...

type ReadonlyStoreApi<T> =
  Pick<StoreApi<T>, "getState" | "subscribe">

And then remove the constraint on S in WithReact (it's an internal type so it's okay) and then instead of WithReact<StoreApi<unknown>> we'd write WithReact<ReadonlyStoreApi<unknown>>. And same change for implementation type WithReact<StoreApi<TState>>.

[lukesmurray]

sounds good I'll update the PR.

[dai-shi]

Does this https://tsplay.dev/wQYPvw still work with the change?

[lukesmurray]

yes. i'll upload an example stackblitz with that as well or can add it to the typings test if you prefer.

[lukesmurray]

the proposed change only enforces the type of getState and subscribe. All other properties are passed through, including setState and other modifications to the store from other middleware.

[dai-shi]

I see, it's the base type (extends ...). Yeah, please add tests.

[devanshj]

Does this https://tsplay.dev/wQYPvw still work with the change?

Yes, we're not changing the create type.

[devanshj]

Let's also widen the constraint on S in UseBoundStore to a readonly store.

And perhaps we should widen the deprecated overload on create.

[dai-shi]

@lukesmurray Can you please follow @devanshj 's suggestions?

[lukesmurray]

yes i’ll submit tomorrow.

…

On Wed, Mar 1, 2023 at 8:08 PM Daishi Kato ***@***.***> wrote: ***@***.**** requested changes on this pull request. @lukesmurray <https://github.com/lukesmurray> Can you please follow @devanshj <https://github.com/devanshj> 's suggestions? — Reply to this email directly, view it on GitHub <#1589 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIDRW4VB3C6KWYNGE3RGFRLWZ7XJDANCNFSM6AAAAAAUMQT3SQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[lukesmurray]

@dai-shi I've updated the PR. thank you for your patience

[dai-shi]

LGTM! Thanks for working on it.

Will merge when we prepare a next release.