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): revert types of redux middleware #1206

Merged
merged 2 commits into from Aug 22, 2022

Conversation

dai-shi
Copy link
Member

@dai-shi dai-shi commented Aug 19, 2022

In #1144, I made some types loosen, which includes not only State but also Action in the redux middleware. @devanshj pointed out that it can cause misusage with the combination of redux and devtools middleware.

So, I'm convinced to revert the Action part of the change.

reducer: (state: T, action: A) => T,
initialState: T
) => StateCreator<Write<T, ReduxState<A>>, Cms, [['zustand/redux', A]]>

declare module '../vanilla' {
interface StoreMutators<S, A> {
'zustand/redux': WithRedux<S, A>
'zustand/redux': WithRedux<S, A & Action>
Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't so sure, but is this what you mean? @devanshj

Copy link
Contributor

@devanshj devanshj Aug 20, 2022

Choose a reason for hiding this comment

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

Yes.

Bonus if you use Cast instead. To see the difference write a store with redux middleware, hover on the dispatch function, you'll see in the action parameter there's an extra and redundant & Action, but if you use Cast the action type will be as it is. If you want I can also explain how Cast works for the record.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I know what Cast does, but I don't get how it improves tooltip. Can this be explained with TS playground?

Copy link
Contributor

@devanshj devanshj Aug 21, 2022

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I understand this one, and why Cast is preferable.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 19, 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 b11f3ac:

Sandbox Source
React Configuration
React Typescript Configuration
React Browserify Configuration
React Snowpack Configuration
React Parcel Configuration

@dai-shi dai-shi mentioned this pull request Aug 19, 2022
9 tasks
@github-actions
Copy link

github-actions bot commented Aug 19, 2022

Size Change: -329 B (-1%)

Total Size: 29.1 kB

Filename Size Change
dist/esm/middleware.js 2.73 kB -71 B (-3%)
dist/middleware.js 3.24 kB -64 B (-2%)
dist/system/middleware.development.js 2.82 kB -72 B (-2%)
dist/system/middleware.production.js 1.75 kB -32 B (-2%)
dist/umd/middleware.development.js 3.35 kB -58 B (-2%)
dist/umd/middleware.production.js 2.06 kB -32 B (-2%)
ℹ️ View Unchanged
Filename Size
dist/context.js 518 B
dist/esm/context.js 464 B
dist/esm/index.js 426 B
dist/esm/middleware/immer.js 210 B
dist/esm/shallow.js 272 B
dist/esm/vanilla.js 402 B
dist/index.js 670 B
dist/middleware/immer.js 373 B
dist/shallow.js 318 B
dist/system/context.development.js 589 B
dist/system/context.production.js 393 B
dist/system/index.development.js 602 B
dist/system/index.production.js 394 B
dist/system/middleware/immer.development.js 292 B
dist/system/middleware/immer.production.js 187 B
dist/system/shallow.development.js 338 B
dist/system/shallow.production.js 253 B
dist/system/vanilla.development.js 471 B
dist/system/vanilla.production.js 285 B
dist/umd/context.development.js 662 B
dist/umd/context.production.js 488 B
dist/umd/index.development.js 816 B
dist/umd/index.production.js 570 B
dist/umd/middleware/immer.development.js 514 B
dist/umd/middleware/immer.production.js 371 B
dist/umd/shallow.development.js 447 B
dist/umd/shallow.production.js 344 B
dist/umd/vanilla.development.js 607 B
dist/umd/vanilla.production.js 390 B
dist/vanilla.js 475 B

compressed-size-action

@dai-shi dai-shi added this to the v4.1.1 milestone Aug 19, 2022
@dai-shi
Copy link
Member Author

dai-shi commented Aug 20, 2022

@devanshj How's this change? b11f3ac

@devanshj
Copy link
Contributor

@devanshj How's this change? b11f3ac

It'd kinda work because all those types are not exported, so we need not worry much about being correct. However I personally would still be correct even for internal types and keep the constraint.

@dai-shi
Copy link
Member Author

dai-shi commented Aug 22, 2022

@devanshj How's this change? b11f3ac

It'd kinda work because all those types are not exported, so we need not worry much about being correct. However I personally would still be correct even for internal types and keep the constraint.

I kinda like what I did. In my mental model, the redux middleware internal types are based on Action extends unknown because there're no restrictions. The public API is based on Action extends { type: unknown }, because it's required by devtools middleware.

So, unless we have obvious limitations like quickinfo one, I'd go with this.

@devanshj
Copy link
Contributor

Yeah that mental model is not bad, so yeah it's okay to go with this.

@dai-shi dai-shi merged commit ca0786f into main Aug 22, 2022
@dai-shi dai-shi deleted the fix/middleware/redux/revert-410-types branch August 22, 2022 01:57
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.

None yet

2 participants