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

Introduce TypedStore<State,Action>! #125

Merged
merged 2 commits into from
Jan 29, 2023
Merged

Conversation

mpetuska
Copy link
Contributor

Closes #92
Closes #46

@mpetuska mpetuska marked this pull request as draft January 18, 2023 19:49
@mpetuska
Copy link
Contributor Author

Some additional tests are still missing but public API is final.

@patjackson52
Copy link
Contributor

I'm not following why adding a type for action to Store. What is the need this is fulfilling? To require a common subclass for all actions? I'm in favor of not requiring a type for actions. Most stores would use Any as action type and requiring a common subclass is just adding complexity & confusion imo.

The ReducerForAction type was used along with some other code as a way reduce type checking in reducers and make them more concise. I think a full example of how it was used is lacking. Not sure this is even a real issue.

@mpetuska
Copy link
Contributor Author

To be clear, this feature set is entirely opt in. The benefits are typesafe dispatchers and reducers. The downsides are that some enchancers might become unavailable (e.g. thunk). In any case, traditional stores and reducers remain exactly the same. I'll bring back removed typealiases with deprecation in the next patch to this.

I personally missed typesafe store in some of my projects as well.

@mpetuska mpetuska force-pushed the reducer-for-action-enchancement branch from d304b15 to 9844d5b Compare January 21, 2023 19:54
@mpetuska mpetuska marked this pull request as ready for review January 21, 2023 19:54
@mpetuska mpetuska enabled auto-merge (rebase) January 21, 2023 19:54
@mpetuska
Copy link
Contributor Author

@patjackson52 PR is ready for review. Here are a few things I'd like you to double-check while reviewing:

  • No existing API changes (i.e. old code will fully compile with new code)
  • New Typed entities are entirely opt-in
  • Two deprecated entities make sense and looks correct to work with Intellij auto-replace intents.

@mpetuska mpetuska disabled auto-merge January 22, 2023 21:45
@mpetuska mpetuska enabled auto-merge (rebase) January 25, 2023 13:05
Copy link
Contributor

@patjackson52 patjackson52 left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks for reworking and keeping original api unchanged. Addition of TypedStore seems to be requested enough that should be included in the lib.
Thanks for putting this up!

@mpetuska mpetuska force-pushed the reducer-for-action-enchancement branch 2 times, most recently from 440bd7e to 82d3ade Compare January 29, 2023 12:16
@mpetuska mpetuska force-pushed the reducer-for-action-enchancement branch from 82d3ade to 0a6c130 Compare January 29, 2023 12:21
@mpetuska mpetuska merged commit b01a7a8 into master Jan 29, 2023
@mpetuska mpetuska deleted the reducer-for-action-enchancement branch January 29, 2023 12:48
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.

createThreadSafeStore doesn't support ReducerForActionType ReducerForAction not usable with current store
2 participants