Add reselect and redux actions to organizations #269

alejandronanez · 2017-08-19T16:04:26Z

Hey there!
~~I have a WIP branch with some updates on our codebase, it is still not done but I want to get early feedback on what I have here.~~

~~Will update this description as I make more progress - thanks for your patience!~~

Update

I completed my work on this but I decided to not include Immutable (not in this PR at least), because reselect + redux actions can live without Immutable.

So, the structure of the code has changed slightly, it goes like:

We have a organization.selectors.js file. Here we're using reselect https://github.com/reactjs/reselect to "query" our store and get the data we need (reselect also has a bunch of benefits that you can read in their docs). This will save us some duplication when we want to get data from the store in different containers (we should reuse the selectors we have in the selectors file whenever we want to use an specific part of the state)
We have a organization.constants.js file, I took this approach because we can use the constants through our reducers and actions (and in our tests in the future) also, I find the *.type.js notation quite ambiguous
We no longer have that nasty switch statement in our reducer (I love this part)
We have individual actions that take care of specific parts of the state, for example we can call an action just to update the loading state of the organization repos or update the error state only on the organization members - No more "batch" updates to the store (What I mean by that is having actions that takes care of 2+ things in our store) This will give us way more control over how our store is being updated.

alejandronanez · 2017-08-19T16:06:03Z

src/organization/organization.selectors.js

+import { createSelector } from 'reselect';
+import { getOrganization as getOrganizationFromStore } from '../../root.reducer';
+
+export const getOrganization = createSelector(


Benefits of using reselect instead of querying the store by.doing.something.like.this

Selectors can compute derived data, allowing Redux to store the minimal possible state. Selectors are efficient. A selector is not recomputed unless one of its arguments change. Selectors are composable. They can be used as input to other selectors.

More info https://github.com/reactjs/reselect

alejandronanez · 2017-08-19T16:06:35Z

src/organization/screens/organization-profile.screen.js

+  getOrganizationIsPendingMembers,
+} from '../index';
+
+const selectors = createStructuredSelector({


This will be almost the same as mapStateToProps but it uses reselect.

alejandronanez · 2017-08-19T16:07:17Z

src/organization/organization.type.js

@@ -1,5 +0,0 @@
-import { createActionSet } from 'utils';
-
-export const GET_ORG = createActionSet('GET_ORG');


I got rid of this *.type.js file in favor of *.constants.js and *.actions.js files

Nice this is perfect, using redux-actions looks like we don't need a helper utility like createActionSet whatsoever

alejandronanez · 2017-08-19T16:07:37Z

src/organization/organization.reducer.js

 };

-export const organizationReducer = (state = initialState, action = {}) => {
-  switch (action.type) {


Got rid of this ugly switch statement.

OMG how have I not used redux-actions before, it looks so much cleaner 😍

alejandronanez · 2017-08-19T16:08:03Z

src/organization/organization.reducer.js

@@ -7,63 +19,65 @@ const initialState = {
  isPendingOrg: false,
  isPendingRepos: false,
  isPendingMembers: false,
-  error: '',
+  organizationError: '',


Created one error for organization, another for repositories and another for members.

alejandronanez · 2017-08-19T16:08:24Z

src/organization/organization.record.js

@@ -0,0 +1,3 @@
+import { Record } from 'immutable';
+
+// WIP


Immutable work should go here =)

alejandronanez · 2017-08-19T16:09:43Z

src/organization/organization.action.js


-    dispatch({ type: GET_ORG.PENDING });
+export const getOrg = createAction(GET_ORG);


Creating actions by using redux-actions I think this is more organized than have a manual dispatch with some boilerplate like {type: ... , payload: ...}

Nice agreed, makes things look so much more oganized 👍

alejandronanez · 2017-08-19T16:09:59Z

root.reducer.js

@@ -16,3 +16,7 @@ export const rootReducer = combineReducers({
  search: searchReducer,
  notifications: notificationsReducer,
 });
+
+// Used to get organization information in selectors
+export const getOrganization = state => state.organization;


This is used in the selectors to get some data.

housseindjirdeh

This looks so good @alejandronanez. Can't explain how much things are already looking cleaner.

Just left a few discussion points, but otherwise I can't see anything I would change outright.

housseindjirdeh · 2017-08-20T16:45:19Z

src/organization/organization.reducer.js

 };

-export const organizationReducer = (state = initialState, action = {}) => {
-  switch (action.type) {


OMG how have I not used redux-actions before, it looks so much cleaner 😍

housseindjirdeh · 2017-08-20T16:52:37Z

src/organization/organization.type.js

@@ -1,5 +0,0 @@
-import { createActionSet } from 'utils';
-
-export const GET_ORG = createActionSet('GET_ORG');


Nice this is perfect, using redux-actions looks like we don't need a helper utility like createActionSet whatsoever

housseindjirdeh · 2017-08-20T16:53:50Z

src/organization/organization.action.js


-    dispatch({ type: GET_ORG.PENDING });
+export const getOrg = createAction(GET_ORG);


Nice agreed, makes things look so much more oganized 👍

housseindjirdeh · 2017-08-20T17:00:27Z

src/organization/organization.selectors.js

+
+export const getOrganization = createSelector(
+  getOrganizationFromStore,
+  organization => organization.organization || {}


I was having a disussion with colleagues from work this past week and a few were under the impression that when simply getting object parameters with dot notation doesn't necessarily need createSelector. For example, this would be sufficient:

export const getOrganization = state => state.organization.organization;

Not necessarily sure if I agree however. Majority of all our selectors are simple getters and not involve manipulation of data in anyway so it seems if we went with this approach we wouldn't really be using createSelector for the majority of our state.

Just wanted to get your input on this mate. Do you usually use createSelector for all data or only if you're retrieving data and performing manipulation to it?

Hey dude! I usually use it to query the store and do some computations based on that data when needed. The big plus I see is that we can compose any selectors we want, for example combine one selector from organizations, one from repos and two from auth, then produce a new prop to pass to any component we want. I truly believe this is a good adition to the stack.

Here's a copy paste from reselect's site

Selectors can compute derived data, allowing Redux to store the minimal possible state. Selectors are efficient. A selector is not recomputed unless one of its arguments change. Selectors are composable. They can be used as input to other selectors.

housseindjirdeh

Thanks a million again for this <3<3<3<3<3<3<3<3

Should we have a separate issue open to track adding reselect and redux-actions to all the other parts of the app?

alejandronanez · 2017-08-21T11:16:56Z

@housseindjirdeh yes! I think that's the right approach! 🙌🏼

Alejandro Nanez and others added 6 commits August 7, 2017 21:25

Adding Immutable - Reselect - Redux Actions

8031126

Merge branch 'master' of github.com:gitpoint/git-point into immutable

0627705

Initial refactor for reducer / actions / constants

da0d703

Merge branch 'master' of github.com:gitpoint/git-point into immutable

8ecc214

Add reselect into organizations

ee8f821

More refactor´

9c7970f

alejandronanez added the 🏗️ work in progress label Aug 19, 2017

alejandronanez self-assigned this Aug 19, 2017

alejandronanez requested review from RolfKoenders, patw0929, lex111, junedomingo, Antoine38660, andrewda and housseindjirdeh August 19, 2017 16:04

alejandronanez commented Aug 19, 2017

View reviewed changes

andrewda changed the title ~~Reselect + Redux Actions + Immutable - WIP~~ wip: add reselect, redux actions and immutable Aug 19, 2017

alejandronanez added 3 commits August 19, 2017 16:59

Fixing base selector errors

800fb1e

Completed refactor for Organization's code

37f1d83

Fix eslint error

3e0e038

alejandronanez changed the title ~~wip: add reselect, redux actions and immutable~~ wip: add reselect and redux actions Aug 19, 2017

alejandronanez changed the title ~~wip: add reselect and redux actions~~ Add reselect and redux actions Aug 19, 2017

alejandronanez added discussion enhancement ready for review refactor and removed 🏗️ work in progress labels Aug 19, 2017

Removing immutable

a08f0b4

alejandronanez mentioned this pull request Aug 20, 2017

PoC for Immutable - Reselect #187

Closed

housseindjirdeh reviewed Aug 20, 2017

View reviewed changes

andrewda approved these changes Aug 20, 2017

View reviewed changes

housseindjirdeh changed the title ~~Add reselect and redux actions~~ Add reselect and redux actions to organizations Aug 21, 2017

housseindjirdeh approved these changes Aug 21, 2017

View reviewed changes

housseindjirdeh merged commit 74bdd70 into master Aug 21, 2017

alejandronanez deleted the immutable branch August 21, 2017 11:15

This was referenced Oct 13, 2017

refactor(redux): Switch to redux middleware & normalizr for Rest #457

Closed

Discussion - Move to Redux Actions + Reselect #520

Closed

		@@ -1,5 +0,0 @@
		import { createActionSet } from 'utils';

		export const GET_ORG = createActionSet('GET_ORG');


		dispatch({ type: GET_ORG.PENDING });
		export const getOrg = createAction(GET_ORG);

Add reselect and redux actions to organizations #269

Add reselect and redux actions to organizations #269

Uh oh!

Conversation

alejandronanez commented Aug 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Update

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

housseindjirdeh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

housseindjirdeh left a comment

Choose a reason for hiding this comment

Uh oh!

alejandronanez commented Aug 21, 2017

Uh oh!

Uh oh!

alejandronanez commented Aug 19, 2017 •

edited

Loading