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

feat: Migrations regarding higher kinded mutators for v4 #772

Merged
merged 16 commits into from
Feb 10, 2022

Conversation

devanshj
Copy link
Contributor

@devanshj devanshj commented Feb 2, 2022

(#715 branch renamed)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 2, 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 1ef2049:

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

@devanshj
Copy link
Contributor Author

devanshj commented Feb 2, 2022

In #715 you reviewed and finally we were going to remove PartialState, StateSliceListener and Destroy. Let's keep Destroy just to be consistent as we have types for all other keys of store.

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.

Looks like a good migration path!

@dai-shi
Copy link
Member

dai-shi commented Feb 9, 2022

So, this looks almost ready.
Can you fix remaining issues and CI?

@dai-shi dai-shi added this to the v3.7.0 milestone Feb 9, 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.

Okay, I'm merging this for v3.7.0. If we have more ideas for migration paths, we can release patch versions v3.7.x.

@devanshj
Copy link
Contributor Author

devanshj commented Feb 9, 2022

Okay, I'm merging this for v3.7.0. If we have more ideas for migration paths, we can release patch versions v3.7.x.

If you're in a hurry go for it otherwise I was about to deprecate other middleware store types in favor Mutate too, we're already deprecating for subscribeWithSelector so it'd be nice if we do it for other middlewares too in one release itself.

@dai-shi
Copy link
Member

dai-shi commented Feb 9, 2022

If you're in a hurry go for it otherwise I was about to deprecate other middleware store types in favor Mutate too, we're already deprecating for subscribeWithSelector so it'd be nice if we do it for other middlewares too in one release itself.

Oh, I thought it's impossible.
It would be nice if you can. I'll wait for it.

@devanshj devanshj requested a review from dai-shi February 9, 2022 13:23
@devanshj
Copy link
Contributor Author

devanshj commented Feb 9, 2022

Right now the deprecation message is this...

/**
 * @deprecated Use `Mutate<StoreApi<T>, [["zustand/subscribeWithSelector", never]]>`.
 * If you have multiple middlewares see the documentation for `Mutate` usage.
 */

I think we need to educate users how to use Mutate for multiple middlewares (see tests), should we do it in the readme? Or in the release notes? (in which case I'd replace "documentation" with "release notes for v3.7.0")

And I think we should also mention this is for an easier migration to v4.

@dai-shi
Copy link
Member

dai-shi commented Feb 9, 2022

yeah, we should write it in release notes (i don't plan to change readme for this.)
if you have a suggestion how to write in the release notes please let me know your draft.

And it may sound crazy, but in the @deprecated message, let's say "See tests/middlewareTypes.test.tsx" and would you modify multiple middleware usage there?

@devanshj
Copy link
Contributor Author

devanshj commented Feb 9, 2022

And it may sound crazy, but in the @deprecated message, let's say "See tests/middlewareTypes.test.tsx" and would you modify multiple middleware usage there?

I'd be fine with that, the readme also anyway tells users to refer to the test file regarding middleware types, so it's not a big deal I guess. And I already have migrated the all tests (including ones with multiple middlewares) to Mutate.

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.

Cool. I'm already pretty happy with this version.

Please follow the coding convention and fix conflicts, and we should be good to go!

@devanshj devanshj requested a review from dai-shi February 10, 2022 13:40
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.

Looks great! Let's ship it.

@dai-shi dai-shi merged commit a418fd7 into pmndrs:main Feb 10, 2022
@dai-shi dai-shi changed the title Migrations regarding higher kinded mutators for v4 feat: Migrations regarding higher kinded mutators for v4 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.

2 participants