Implement `reselect` recommendations and best practices #2823

joeyparis · 2019-11-25T22:32:17Z

This implements the reselect recommendations in this comment. It is one of the To Dos on #2812.

I believe I have fully refactored the usage of reselect in the containers, templates, generators, and tests per @markerikson's recommendations. Test coverage and success remains at 100%, although I will be honest I am relatively new at test suites, and someone should probably double-check my work.

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
-- Not relevant for the nature of this pull request
[✔️] 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.

- Merged App and HomePage slices - RepoListItem becomes a component since we determine ownership of repo in the saga - Delete all useless actions/constants/reducer files - Update all broken tests - Add ESLint exception for reducer files

- Add tests to improve coverage

* Update generators for switch to Redux Toolkit * Remove useless comments from tests * Turn multiline comment into many single line comments ;)

…nents (react-boilerplate#2814)

# Conflicts: # app/containers/App/index.js # app/containers/LanguageProvider/reducer.js # app/containers/LocaleToggle/index.js # internals/templates/containers/LanguageProvider/reducer.js # package.json

coveralls · 2019-11-25T22:36:19Z

Coverage remained the same at 100.0% when pulling 6bd8e69 on joeyparis:redux-toolkit-switch into 764a9b8 on react-boilerplate:v5-with-cra.

julienben · 2019-11-25T22:47:00Z

Thanks a lot for implementing this @joeyparis! At first glance, it looks like exactly what was needed. Will review in full tomorrow. Also, we might want to merge this into dev directly after #2812 as the squash merge will overwrite your contribution history.

justingreenberg · 2019-11-25T22:53:56Z

app/containers/App/selectors.js

-    selectRouter,
-    routerState => routerState.location,
-  );
+const selectLocation = createSelector(


hi @joeyparis - is there a reason you removed the closures?

we wrap selectors in factories so that each selector instance maintains a private cache for memoization when using across different components, see:

https://github.com/reduxjs/reselect#sharing-selectors-with-props-across-multiple-component-instances

Hi @justingreenberg, I was really strictly following this comment, which was one of the To Do's. So honestly I can't say I put any deep thought into the use of factories or lack thereof. If you feel there's an alternative way to use factories, and align with the to-do/comment I'm all ears!

Right, and per my comments, the existing code is doing that unnecessarily. There's no reason to have multiple selector instances all reading the exact same single value. The factory pattern is only needed when you will be calling the "same" selector with varying inputs, such as const todo = selectTodoById(state, ownProps.id). In that case, the inconsistent parameters would cause a single selector instance to not memoize at all, so you need separate instances (usually per component).

I talked about this in more detail here:

https://blog.isquaredsoftware.com/2017/12/idiomatic-redux-using-reselect-selectors/

joeyparis

Thanks a lot for implementing this @joeyparis! At first glance, it looks like exactly what was needed. Will review in full tomorrow. Also, we might want to merge this into dev directly after #2812 as the squash merge will overwrite your contribution history.

Awesome, sounds like a plan! Just let me know if I need to change which branch it's being merged into.

joeyparis · 2019-11-25T22:39:25Z

app/containers/HomePage/index.js

-const stateSelector = createStructuredSelector({
-  username: makeSelectUsername(),
-  repos: makeSelectRepos(),
-  loading: makeSelectLoading(),
-  error: makeSelectError(),
-});


I opted for removing createStructuredSelector because of the consideration for removing reselect as a dependency. It does lead to using 4 separate useSelector hooks rather than one deconstructed one. I'm not sure if this is desirable or not, so I'm definitely up for discussing other alternatives!

I think that's correct. You could also remove reselect from package.json as well.

joeyparis · 2019-11-25T23:00:03Z

app/containers/App/selectors.js

-    selectRouter,
-    routerState => routerState.location,
-  );
+const selectLocation = createSelector(


Hi @justingreenberg, I was really strictly following this comment, which was one of the To Do's. So honestly I can't say I put any deep thought into the use of factories or lack thereof. If you feel there's an alternative way to use factories, and align with the to-do/comment I'm all ears!

julienben · 2019-11-27T11:53:05Z

To me this looks good.

@justingreenberg Do you still have objections or are you satisfied by the explanation given by @markerikson? Will wait for your reply to do anything about this branch.

And in any case, this will go directly into dev only after the Redux Toolkit switch is done. (I'm making progress on the documentation updates.)

gretzky

This LGTM as well

justingreenberg · 2019-11-27T13:31:35Z

@joeyparis thanks for clarifying 👍🏼 @julienben yes LGTM! 😊

joeyparis · 2019-11-27T15:11:40Z

Do I need to do anything at this time? Or are we just waiting until the Redux Toolkit switch is done? Thanks for the feedback everyone!

julienben · 2019-11-27T15:35:08Z

Nope, nothing to do I believe.

…

On Wed, Nov 27, 2019 at 5:11 PM joeyparis ***@***.***> wrote: Do I need to do anything at this time? Or are we just waiting until the Redux Toolkit switch is done? Thanks for the feedback everyone! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2823?email_source=notifications&email_token=ACEITH6NDMX5WS2Q4VV5EULQV2E23A5CNFSM4JRPVKUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFJZYHY#issuecomment-559127583>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACEITH433YR724A2Q4ASV4TQV2E23ANCNFSM4JRPVKUA> .

# Conflicts: # app/containers/App/selectors.js # package.json

# Conflicts: # .all-contributorsrc # app/containers/App/selectors.js # app/containers/HomePage/selectors.js # app/containers/LanguageProvider/index.js # app/containers/LanguageProvider/selectors.js # app/containers/LocaleToggle/index.js # internals/templates/containers/App/selectors.js # internals/templates/containers/LanguageProvider/index.js # internals/templates/containers/LanguageProvider/selectors.js

Can-Sahin · 2020-03-10T08:25:03Z

In app/containers/ReposManager/selectors.js did you forget to import createSelector from @reduxjs/toolkit ? Because reselect isn't need anymore

// import { createSelector } from 'reselect'; // INSTEAD
import { createSelector } from '@reduxjs/toolkit';

joeyparis · 2020-03-10T14:46:29Z

@Can-Sahin looks like I did. Fixed and updated!

pedro-victor

Apart from the .all-contributorsrc and removing reselect from package.json, I think this PR does basically what I have been using on my own projects (which I mistakenly pushed #2921 ) and looks to be in line with the recommendations.

pedro-victor · 2020-05-25T17:49:56Z

.all-contributorsrc

+      "contributions": [
+        "code"
+      ]
+    },


I believe this modification breaks the file.

pedro-victor · 2020-05-25T17:50:49Z

app/containers/HomePage/index.js

-const stateSelector = createStructuredSelector({
-  username: makeSelectUsername(),
-  repos: makeSelectRepos(),
-  loading: makeSelectLoading(),
-  error: makeSelectError(),
-});


I think that's correct. You could also remove reselect from package.json as well.

Can-Sahin · 2020-07-03T13:15:14Z

Merging due to the major v5 release. Read the discussion here

Can-Sahin · 2020-07-12T11:31:17Z

Edit: Sorry for merging this somewhere else due to #2935. If you want please re submit this PR to the dev branch if that means anything to you

markerikson and others added 30 commits November 21, 2019 10:46

Add Redux Toolkit

1d8b7d4

Remove explicit dependencies on packages used by RTK

1b29294

Replace references to plain Redux package

ae78600

Convert store setup to use configureStore from RTK

5c558e4

Replace references to plain Reselect library

15d390d

Add an initial app slice

3f924ef

Update other uses of app slice actions

026e7aa

Fix lint errors in appSlice

f0dd5d1

Fix mismatched argument in saga test

6ac2663

Fix incorrect preloadedState argument for configureStore

849aadc

Remove obsolete configureStore test

6dc9d93

Fix broken initial state in template store setup

54fff0b

Fix broken test

0cf7263

Use appSlice reducer in application

7c7375c

- Fix and refactor sample app:

ec1cf41

- Merged App and HomePage slices - RepoListItem becomes a component since we determine ownership of repo in the saga - Delete all useless actions/constants/reducer files - Update all broken tests - Add ESLint exception for reducer files

Group HomePage components into one subfolder for cleanliness

4ee4642

- Remove empty if from configureStore

b4bd59e

- Add tests to improve coverage

Update the LangugageProvider in sample and template apps

f50f930

Fix LocaleToggle test

241e0c6

Remove Mensae from CODEOWNERS

d3d0f3b

Rename some tests

7a7d7bd

Move repo management to ReposManager

3618574

[REDUX TOOLKIT] Update generators (react-boilerplate#2811)

4d6fd9a

* Update generators for switch to Redux Toolkit * Remove useless comments from tests * Turn multiline comment into many single line comments ;)

Fix HomePage tests after RTK update and add tests for uncovered compo…

fe41c35

…nents (react-boilerplate#2814)

Merge branch 'dev' into redux-toolkit-switch

24a8fcc

# Conflicts: # app/containers/App/index.js # app/containers/LanguageProvider/reducer.js # app/containers/LocaleToggle/index.js # internals/templates/containers/LanguageProvider/reducer.js # package.json

Implement reselect recommendations and best practices for containers

4fec721

Update tests for new reselect implementation

3d1c34c

Update template containers for new reselect implementation

c644230

Update generators for new reselect implementation

bbeb51f

Update redux documentation for new reselect implementation

9812002

JoeyLeadJig added 2 commits November 25, 2019 17:04

Add @joeyparis as a code contributor

2a06bd6

Fix improper deconstruction of selectLocale selector

26617ba

joeyparis requested review from gretzky, julienben, justingreenberg and jwinn as code owners November 25, 2019 22:32

justingreenberg reviewed Nov 25, 2019

View reviewed changes

joeyparis commented Nov 25, 2019

View reviewed changes

gretzky approved these changes Nov 27, 2019

View reviewed changes

Merge branch 'dev' into redux-toolkit-switch

c252e9d

# Conflicts: # app/containers/App/selectors.js # package.json

julienben mentioned this pull request Dec 3, 2019

Components relying on locale state aren't fully tested to account for undefined value #2824

Open

joeyparis mentioned this pull request Dec 6, 2019

Language provider test improvement #2832

Merged

JoeyLeadJig added 2 commits December 6, 2019 12:40

Fix LanguageProvider template to match initial file

e20ae1c

Replace createSelector import from reselect with @reduxjs/toolkit

8c24a61

pedro-victor mentioned this pull request May 25, 2020

Implement simplified selector pattern #2921

Closed

pedro-victor reviewed May 25, 2020

View reviewed changes

Can-Sahin changed the base branch from redux-toolkit-switch to v5-with-cra July 3, 2020 13:14

Merge branch 'v5-with-cra' into redux-toolkit-switch

6bd8e69

Can-Sahin merged commit fe5198f into react-boilerplate:v5-with-cra Jul 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `reselect` recommendations and best practices #2823

Implement `reselect` recommendations and best practices #2823

joeyparis commented Nov 25, 2019

coveralls commented Nov 25, 2019 •

edited

Loading

julienben commented Nov 25, 2019

justingreenberg Nov 25, 2019

joeyparis Nov 25, 2019

markerikson Nov 25, 2019

joeyparis left a comment

joeyparis Nov 25, 2019

pedro-victor May 25, 2020

joeyparis Nov 25, 2019

julienben commented Nov 27, 2019

gretzky left a comment

justingreenberg commented Nov 27, 2019

joeyparis commented Nov 27, 2019

julienben commented Nov 27, 2019 via email

Can-Sahin commented Mar 10, 2020

joeyparis commented Mar 10, 2020

pedro-victor left a comment

pedro-victor May 25, 2020

pedro-victor May 25, 2020

Can-Sahin commented Jul 3, 2020

Can-Sahin commented Jul 12, 2020

Implement reselect recommendations and best practices #2823

Implement reselect recommendations and best practices #2823

Conversation

joeyparis commented Nov 25, 2019

React Boilerplate

coveralls commented Nov 25, 2019 • edited Loading

julienben commented Nov 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyparis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienben commented Nov 27, 2019

gretzky left a comment

Choose a reason for hiding this comment

justingreenberg commented Nov 27, 2019

joeyparis commented Nov 27, 2019

julienben commented Nov 27, 2019 via email

Can-Sahin commented Mar 10, 2020

joeyparis commented Mar 10, 2020

pedro-victor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Can-Sahin commented Jul 3, 2020

Can-Sahin commented Jul 12, 2020

Implement `reselect` recommendations and best practices #2823

Implement `reselect` recommendations and best practices #2823

coveralls commented Nov 25, 2019 •

edited

Loading