Feature/add redux toolkit

[markerikson]

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]

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]

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.

[gretzky]

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.

[markerikson]

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:

react-boilerplate/app/containers/App/reducer.js

Lines 23 to 30 in 27538f1

	/* 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.

[julienben]

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.

[gretzky]

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.

[justingreenberg]

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 😄

[markerikson]

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.

[julienben]

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.

[markerikson]

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

[julienben]

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.

[julienben]

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]

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]

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]

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]

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

[julienben]

See #2808 for ongoing work into this.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.