-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
make type NamedSet compatible with type SetState #441
Conversation
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 ad16a4e:
|
K3 extends keyof T = K2, | ||
K4 extends keyof T = K3 | ||
>( | ||
partial: PartialState<T, K1, K2, K3, K4>, |
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.
This change looks good.
I updated the test file. Not sure if we can combine I think something like this right? const createStoreWithImmer = <T extends State>(
createState: TImmerConfig<T>,
prefix = 'prefix',
persistName = 'persist'
): UseStore<T> & ISelectors<T> => {
return createSelectorHooks(
create(
devtools(persist(immer(createState), { name: persistName }), prefix)
)
)
} |
@TkDodo Do you think OK to merge this? Can review? |
I tried to solve other issues, but still no luck. Will update it if I can solve any issue |
@zgid123 This PR itself is ready for review and merge, right? |
Yup, this is for #434 |
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.
Thanks, everyone.
According to #434, we are having issue with typescript when combining devtool with persist middleware.
The main issue is because NamedSet and SetState are not compatible, so I think we can temporary make them compatible.
I already added some codes to test the type for middleware use case only. And also add
immer
asdevDependency
for testing (not sure this is a way, I will remove if it's not suitable).