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

Feature/add redux toolkit #2807

Merged

Conversation

@markerikson
Copy link

markerikson commented Nov 18, 2019

React Boilerplate

Thank you for contributing! Please take a moment to review our contributing guidelines
to make the process easy and effective for everyone involved.

Please open an issue before embarking on any significant pull request, especially those that
add a new library or change existing tests, otherwise you risk spending a lot of time working
on something that might not end up being merged into the project.

Before opening a pull request, please ensure:

  • You have followed our contributing guidelines
  • Double-check your branch is based on dev and targets dev
  • Pull request has tests (we are going for 100% coverage!)
  • Code is well-commented, linted and follows project conventions
  • Documentation is updated (if necessary)
  • Internal code generators and templates are updated (if necessary)
  • Description explains the issue/use-case resolved and auto-closes related issues

Be kind to code reviewers, please try to keep pull requests as small and focused as possible :)

IMPORTANT: By submitting a patch, you agree to allow the project
owners to license your work under the terms of the MIT License.

Details

This PR demonstrates how to convert part of the built-in example application to use Redux Toolkit, as described in #2806 .

It is not meant to be complete and merged as-is, but rather to serve as a starting point for conversion if the maintainers choose.

Changes

  • Added Redux Toolkit
  • Removed the explicit dependency on Redux, since RTK re-exports everything. (I also attempted to remove the Immer and Reselect dependencies, but saw that there were other reducers that currently depend on Immer, and a couple components that depend on Reselect's createStructuredSelector, so I re-added those explicit dependencies.)
  • Converted the store setup logic in configureStore.js to use RTK's configureStore method
  • Added a new containers/App/appSlice.js file that contains equivalent logic to the existing App/reducers.js and App/actions.js files
  • Modified other uses of App/actions to use the action creators exported by appSlice.js
  • Removed obsolete store setup tests that checked for calling the DevTools Extension global API

I have some other changes I'd like to suggest as well, which I'll write up in the issue rather than here.

@markerikson

This comment has been minimized.

Copy link
Author

markerikson commented Nov 18, 2019

Looking at the Travis logs, it appears that the tests themselves are passing, but the global code coverage stats have fallen below a 98% threshold.

@gretzky

This comment has been minimized.

Copy link
Contributor

gretzky commented Nov 18, 2019

Hey it's the redux overlord! Thanks for this, tbh I have no idea what redux-toolkit even is so this will be cool to take a look at.

},
};

/* eslint-disable default-case, no-param-reassign */

This comment has been minimized.

Copy link
@gretzky

gretzky Nov 18, 2019

Contributor

Could you talk more about the way the param reassignment works here? I'm not sure how I feel about turning off a lint rule to accommodate the way a package works. I also think that as I'm reading this, it comes off as if we're mutating state here by reassigning the state params. This might just be me not fully understanding the way this works under the hood.

This comment has been minimized.

Copy link
@markerikson

markerikson Nov 18, 2019

Author

Sure.

RTK specifically uses the Immer library library inside. Immer lets you write "mutating" code, but it wraps the original value in an ES6 Proxy and tracks the modifications, and then safely returns a correctly immutably updated value (as if you'd done all the spreads and stuff by hand).

react-boilerplate is already using Immer in its reducers, and in fact I copied the eslint-disable line from another file, like the equivalent App/reducer.js:

/* eslint-disable default-case, no-param-reassign */
const appReducer = produce((draft, action) => {
switch (action.type) {
case LOAD_REPOS:
draft.loading = true;
draft.error = false;
draft.userData.repositories = false;
break;

So, in that sense there's nothing "new" here for react-boilerplate, this is already stuff you've got in the codebase. The only changes are A) the additional @reduxjs/toolkit package, B) the createSlice() function auto-generating the action creators and action types, and C) consolidating the resulting logic into one file instead of several.

This comment has been minimized.

Copy link
@julienben

julienben Nov 18, 2019

Member

Yes, we had to disable these rules for our migration to immer. You should only need /* eslint-disable no-param-reassign */ here though as default-case is only for switches.

This comment has been minimized.

Copy link
@gretzky

gretzky Nov 18, 2019

Contributor

Word, thanks for the explanations. I like to make my day-to-day life difficult by not really ever using immutability libs, so this is helpful.

This comment has been minimized.

Copy link
@justingreenberg

justingreenberg Nov 18, 2019

Member

lol yes @gretzky i'm with you—super clever implementation but personally not a fan of immer...setting properties using dot notation just feels wrong. long live explicit immutability 😄

This comment has been minimized.

Copy link
@markerikson

markerikson Nov 18, 2019

Author

The good news is that using RTK and Immer doesn't require that you actually use "mutative" logic - you can still return manually written immutable updates.

That said, frankly hand-written immutable update code is overly verbose, hard to read, and very prone to mistakes. I understand the concerns about this "feeling wrong", but using Immer is actually much safer and easier to read.

This comment has been minimized.

Copy link
@julienben

julienben Nov 19, 2019

Member

I don't think there's anything inherently wrong with dot notation for writing immutable updates but hiding immer can IMO make things confusing for new users. They might not realize that they're not actually mutating the state.

I was previously against this when someone suggested switching to redux-immer. I want to make sure that we make it as clear as possible (via docs and/or comments) that there's some work in the bg making it possible to write immutable logic with dot notation.

This comment has been minimized.

Copy link
@markerikson

markerikson Nov 19, 2019

Author

Yeah, the teaching aspect is definitely the hardest part here.

Copy link
Member

julienben left a comment

This looks great!

As you mention, it's a WIP and requires more work before being ready but unless somebody would like to argue against this switch I think we can safely proceed with the additional changes required for it.

@markerikson Would you like to continue this work or would you like local contributors to take it on?

Quick list of things which still need to be done:

  • Obsolete actions/reducers/constants files need to be deleted along with their tests
  • The container generators need to be updated
  • appSlices need to be tested (+ 100% coverage recovered if it still makes sense)
  • Documentation needs to be updated with IMO references to the usage guide
  • Consider if we can actually remove reselect and immer as dependencies. This would make sense since Redux Toolkit provides its own version of each.
  • For myself or anyone else who wants to take it on: Implement the reselect recommendations in this comment after we merge this.
app/configureStore.js Show resolved Hide resolved
app/configureStore.js Show resolved Hide resolved
app/containers/HomePage/saga.js Show resolved Hide resolved
},
};

/* eslint-disable default-case, no-param-reassign */

This comment has been minimized.

Copy link
@julienben

julienben Nov 18, 2019

Member

Yes, we had to disable these rules for our migration to immer. You should only need /* eslint-disable no-param-reassign */ here though as default-case is only for switches.

@markerikson

This comment has been minimized.

Copy link
Author

markerikson commented Nov 18, 2019

I'd like to turn the PR over to you folks at this point. You know the react-boilerplate codebase and requirements way better than I do, and I have my own priorities atm (like rewriting most of the Redux docs).

Happy to answer any questions about using RTK as you work through this, though!

It's likely that you can remove Immer as a direct dependency once the other reducers have been converted. Reselect might not be removable, given that there are uses of createStructuredSelector, which RTK does not re-export. (Technically you can get away with import {createStructuredSelector} from "reselect" at a code level with only having an explicit dependency on RTK, but that's dependent on the bundler + package management tool letting you do that, and there's currently an ESLint rule that yells at you for doing that anyway.)

@justingreenberg

This comment has been minimized.

Copy link
Member

justingreenberg commented Nov 18, 2019

hi @markerikson thank you for this PR!

Reselect might not be removable, given that there are uses of createStructuredSelector, which RTK does not re-export

i don't think it's a huge deal to keep reselect dependency, but would you consider adding createStructuredSelector to RTK exports? maybe in a utility export with other commonly used tools from libs, such as redux's compose?

@markerikson

This comment has been minimized.

Copy link
Author

markerikson commented Nov 18, 2019

We do re-export everything from Redux already.

I'd like to avoid explicitly re-exporting createStructuredSelector for now. If someone really needs it, they can just declare the dependency on Reselect themselves. Since RTK already pulls that in, it's just adding another entry to package.json.

@julienben julienben changed the base branch from dev to redux-toolkit Nov 19, 2019
@julienben

This comment has been minimized.

Copy link
Member

julienben commented Nov 19, 2019

I'm merging this into a React Boilerplate branch called redux-toolkit so we can continue working there.

@julienben julienben merged commit 130bbb5 into react-boilerplate:redux-toolkit Nov 19, 2019
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@julienben julienben mentioned this pull request Nov 19, 2019
3 of 8 tasks complete
@julienben

This comment has been minimized.

Copy link
Member

julienben commented Nov 19, 2019

See #2808 for ongoing work into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.