react-slingshot
react-slingshot copied to clipboard
Add Redux Toolkit
Pull Request Template
The code review checklist below is used for all pull requests.
- Review the list before submitting your pull request.
- Leave the list intact for the code reviewer's use.
Checklist
- [x] Latest code from master has been merged into the pull request branch
- [x] Code is camelCased
- [x] No commented out code (if required, place // TODO above with explanation)
- [x] No linting issues
- [x] Automated tests exist and pass
- [x] Build is successful (
npm run build
) - [x] 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 onredux-thunk
andredux-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
tocreateRootReducer
for clarity - Converted the main container component to use the "object shorthand" form of
mapDispatch
, which is our recommended way to bind action creators.
Coverage increased (+2.8%) to 94.309% when pulling e103dc2acb31f7e405e9c9b2d8cdf26d685ae27f on markerikson:feature/add-redux-toolkit into 3345563212858bb42d8749c8984a596491f933be on coryhouse:master.
Thanks for the PR @markerikson!
LGTM, but as mentioned in the issue, I will let @coryhouse and @kwelch give this a look as well.
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.
@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.
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.
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?
@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.