react-slingshot icon indicating copy to clipboard operation
react-slingshot copied to clipboard

Add Redux Toolkit

Open markerikson opened this issue 4 years ago • 7 comments

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

  • [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 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 avatar Nov 18 '19 01:11 markerikson

Coverage Status

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

coveralls avatar Nov 18 '19 01:11 coveralls

Thanks for the PR @markerikson!

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

nickytonline avatar Nov 20 '19 15:11 nickytonline

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 avatar Nov 20 '19 23:11 kwelch

@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.

markerikson avatar Nov 21 '19 00:11 markerikson

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.

kwelch avatar Nov 21 '19 02:11 kwelch

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 avatar Nov 21 '19 12:11 coryhouse

@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.

markerikson avatar Nov 21 '19 17:11 markerikson