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

fix(middleware): match vanilla StateCreator type in subscribeWithSelector #787

Merged
merged 2 commits into from
Feb 9, 2022

Conversation

toddw
Copy link
Contributor

@toddw toddw commented Feb 8, 2022

Match

zustand/src/vanilla.ts

Lines 50 to 52 in 0ba3c24

CustomSetState = SetState<T>,
CustomGetState = GetState<T>,
CustomStoreApi extends StoreApi<T> = StoreApi<T>

to make subscribeWithSelector more useful with custom states.

Ran into this when trying to use subscribeWithSelector with
custom middleware that uses immer's WritableDraft on SetState

Match
https://github.com/pmndrs/zustand/blob/0ba3c240078cf61792a4b13ff11d774ecca70a0a/src/vanilla.ts#L50-L52
to make subscribeWithSelector more useful with custom states.

Ran into this when trying to use subscribeWithSelector with
custom middleware that uses immer's WritableDraft on SetState
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 8, 2022

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 e39b1ed:

Sandbox Source
React Configuration
React Typescript Configuration
React Browserify Configuration
React Snowpack Configuration
React Parcel Configuration
Next.js Configuration

@toddw toddw changed the title match vanilla StateCreator type match vanilla StateCreator type in middleware/subscribeWithSelector Feb 8, 2022
Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devanshj Does this conflict with v3.7.0 and v4?

@devanshj
Copy link
Contributor

devanshj commented Feb 8, 2022

It does “conflict” with v4 but there already is a conflict without this too, and it won't hurdle with v3.7.0 as it's not changing much.

But the issue I find here is it'd make sense for all middlewares to have similar type, so if you think this is a good change the same change should be made to other middlewares too.

@dai-shi
Copy link
Member

dai-shi commented Feb 8, 2022

@toddw I think it's a rare case that you need to specify only one type to subscribeWithSelector. What's your case?
Do you want to make this change and maybe others, if this is going to live only for a short period?

@dai-shi
Copy link
Member

dai-shi commented Feb 9, 2022

I made changes for all middlewares. Let's merge this for v3.7.0.

@dai-shi dai-shi added this to the v3.7.0 milestone Feb 9, 2022
@toddw
Copy link
Contributor Author

toddw commented Feb 9, 2022

Looks great! Thanks for updating it

@dai-shi dai-shi merged commit e90e8bc into pmndrs:main Feb 9, 2022
@dai-shi dai-shi changed the title match vanilla StateCreator type in middleware/subscribeWithSelector fix(middleware): match vanilla StateCreator type in subscribeWithSelector Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants