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

Add Redux Toolkit #638

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

markerikson
Copy link

Pull Request Template

The code review checklist below is used for all pull requests.

  1. Review the list before submitting your pull request.
  2. Leave the list intact for the code reviewer's use.

Checklist

  • Latest code from master has been merged into the pull request branch
  • Code is camelCased
  • No commented out code (if required, place // TODO above with explanation)
  • No linting issues
  • Automated tests exist and pass
  • Build is successful (npm run build)
  • Works in IE 11, Chrome, Firefox, and Edge

Details

This PR demonstrates how to convert the default example project to use Redux Toolkit, per #637 . It is not necessarily meant to be merged as-is, although if you'd like to use it as a starting point, please do so.

Changes

  • Added @reduxjs/toolkit as a dependency, and removed the explicit dependencies on redux-thunk and redux-immutable-state-invariant (both of which are used internally by RTK)
  • Converted the store setup logic to use RTK's configureStore(). This considerably simplified the store setup logic, removing the need for separate "dev" and "prod" functions.
  • Created a sample fuelSavingsSlice.js that is equivalent to the existing sample reducer + actions + constants files, including porting the existing reducer tests to cover the new slice file.
  • Renamed rootReducer to createRootReducer for clarity
  • Converted the main container component to use the "object shorthand" form of mapDispatch, which is our recommended way to bind action creators.

@markerikson markerikson changed the title Feature/add redux toolkit Add Redux Toolkit Nov 18, 2019
@coveralls
Copy link

coveralls commented Nov 18, 2019

Coverage Status

Coverage increased (+2.8%) to 94.309% when pulling e103dc2 on markerikson:feature/add-redux-toolkit into 3345563 on coryhouse:master.

@nickytonline
Copy link
Collaborator

Thanks for the PR @markerikson!

LGTM, but as mentioned in the issue, I will let @coryhouse and @kwelch give this a look as well.

@kwelch
Copy link
Collaborator

kwelch commented Nov 20, 2019

First, Hey @markerikson!

I think the changes around mapDispatch, is a great. It was glorious when I found out that was possible.

I think the slice work is great, but it does not appear to be integrated.

I think for the nature of this repo, it should be using one approach consistently. To help promote best practices and simplify the on-boarding experience.

Having both approaches could be confusing to new comers, whom are the target demographic for this repo.

@markerikson
Copy link
Author

@kwelch : right, it's not complete. I just wanted to show the basic pattern so that you folks would have that to work from, and you can complete the conversion from there.

@kwelch
Copy link
Collaborator

kwelch commented Nov 21, 2019

Overall, I really like the approach. Slices look amazing and extremely helpful for sharing which is a major pain point. Also the store configuration is much simpler and cleaner.

@coryhouse
Copy link
Owner

coryhouse commented Nov 21, 2019

Hi Mark - Thanks for the PR! I see good stuff in here I like such as the object form of mapDispatchToProps. You said this isn't ready for merge, so I'm unclear: What open issues are there?

@markerikson
Copy link
Author

@coryhouse : per the earlier comment, I didn't try to clean up all the other existing logic, and I wasn't sure if there were other aspects of the codebase that might also need to be changed. The PR itself might be okay to merge, but I wanted to be clear that it wasn't a 100% complete conversion in and of itself.

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.

5 participants