refactor(redux): Switch to redux middleware & normalizr for Rest by machour · Pull Request #457 · gitpoint/git-point

machour · 2017-10-09T03:10:50Z

Hi,

This is a first draft for a big refactoring of our current store implementation.
It relies on a redux middleware & normalizr to organise the store.

I started implementing it in the Organizations section, and I think it looks solid
enough to showcase this refactoring and start a discussion.

Start Reactotron & subscribe to the following reducers :

entities
pagination
errorMessage

Initial state:

Follow the described navigation, and start witnessing your subscriptions changes:

The organization profile screen:

Go to the "GitPoint" organization profile screen.

Two api calls were made: providers/github/repos.js@getById() && providers/github/repos.js@getMembers().

The first API call checked for the org in entities, did not find it, so dispatched an action interpreted by the result middleware.
After the fetch() call in performApiRequest(), the returned JSON was normalized using the orgSchema, and stored by the entities reducers like so :

The second API call, checked for the existence of "gitpoint" in pagination.membersByOrg, did not find it, so dispatched an action. The returned response from Github contained two things:

A JSON array of users, which was reduced by entities using the userSchema:

Pagination information in the Link header, which was parsed and stored by the pagination reducer in pagination.membersByOrg.gitpoint.
Because we specified that we expected an array of userSchema, normalizr was able to store only the usernames of the members in the pagination, instead of the full user information. (this is reason number 1 that normalizr() is awesome)

(I specifically asked for 8 users per page of result to demonstrate the pagination)

By passing the two functions (getById and getMembers) as mapDispatchToProps in the OrganizationProfileScreen, we can define the following mapStateToProps function:

const mapStateToProps = (state, ownProps) => {
  // We only need the organization id, so navigate() calls should be changed to get rid
  // of params.organization. Kept for now to avoid messing up things
  const orgId = ownProps.navigation.state.params.organization.login.toLowerCase();

  // We retrieve the things we're interested in following in state
  const { pagination: { membersByOrg }, entities: { orgs, users } } = state;

  // We initialize the members pagination as Fetching as it will start fetching right away
  // in componentWillMount()
  const membersPagination = membersByOrg[orgId] || {
    ids: [],
    isFetching: true,
  };
  // Here's how we transform our array of usernames to actual users
  const members = membersPagination.ids.map(id => users[id]);

  return {
    orgId,
    members,
    membersPagination,
    org: orgs[orgId],
  };
};

Scrolling the members list

Remember how I said I limited the pagination to 8 members per page? Let's scroll to the right in the avatars list. By listening on onEndReached, we're able to re-trigger a call to getMembers() with an additional parameter this time: getMembers(orgId, { loadMore: true })

This second parameter makes the next call to Github use the URL stored in pagination.memberByOrg.gitpoint.nextPageUrl that appeared in last screenshot, and that asks for the next page of results. New results will be reduced using the same process as earlier, and our usernames array will grow with the new references to users.

As this was the last page of results, nextPageUrl got set to null, and no more apis calls will be made.

In and out of the organization screen

Go back in navigation, then forward to the organization screen again.
Both getById() and getMembers() were called again, but as everything was already in the store, the calls to Github api were blocked. (see getById() code)

Refreshing the organization screen

Unfold entities.orgs.gitpoint and observe the _fetchedAt property. It's a timestamp of the moment we normalized the entity in the schema.

Pull down the screen to refresh. getById(orgId) & getMembers(orgId) will be called again, this time with a forceRefresh option, that will perform an API call to Github, regardless of what we have or not in our store.

Once the response is fetched, the usual reducing/normalization takes places, and the entity is swapped in the store. See how _fetchedAt changed? Pull the screen again. Such amaze. Much wow.

(:warning: I still haven't fully implemented forceRefreshing on paginated list like getMembers(), so its pagination in the store will look off.)

Browsing the organization repositories

Click the number of repos on the organization screen.
This time, we'll be fetching entities.repos entities, and paginating using the reposByOrg entry in the store.

If you use the search, we we'll be using reposBySearch instead, and indexing with the complete query string sent to GitHub. Here's an example searching for site

Moar data

Well, ok, 2 repositories & 12 members are not enough to really enjoy this baby.
Let's navigate to the "search" tab, and search for "ionic", select the first repo in results, and then click on the owner.

You should now be on the Organization page for the "Ionic" team.

Wait. Navigate back to the tab you came from.
Notice how data is still related to gitpoint, instead of being swapped to Ionic?

You can also try and go to 4 different organizations on the 4 tabs, scroll down their repositories, and switch beetween tabs.

One of our major flaws just got fixed ! 🕺

Moar amaze

Schemas are wonderful

By extensively using the processStrategy of normalizr schemas, we are able to craft our stored entities with care to only keep attributes that are used by the app, and minimize the store size (we don't store the dozen of urls that comes with every response from github for example)
We can standardize some useful internal attributes:

You already know about _fetchedAt (that we will be able to use later to leverage conditional request.

There is also _entityUrl which points to the entity on github website. This will be usefull for #285

There is also _isComplete which tells us if the entity was completely or partially received. (For example, the members returned by getMember() are incomplete users, in comparison to what you get on the /user/:username). (Usage still WIP)

Isn't that too much for the store ?

If we're worried about the store growing too much in size, we can add a _lastUsed property.
In getById() for example, when we check the store and find that gitpoint is already in the store, we could dispatch a UPDATE_USED_AT action to update the _lastUsed attribute.

Then, we could implement an other action, triggered on low memory warning, (or when we have more than N entities) to go through entities and remove the old entries.

Not persisted

entities, pagination and errorMessage are blacklisted from the persistent store. Fresh data every time we open the app.

Gitlab? Bitbucket?

With this new redux middleware, and the use of schemas, we could add support for Gitlab & Bitbucket by keeping things standardized.

By implementing provider-specific schemas, and adopting an internal convention for those, we will be able to produce entities with the exact same structure for all 3 providers.

Which means that all screens/components using these entities, can display the data regardless of where they came from.

Of course, for now, our first objective is to bring the Github provider up and 100% making use of this new redux store.

I intendedly added an extra providers/github path in this PR, for us to keep standardization in mind.

Voilà!

alejandronanez · 2017-10-09T04:06:12Z

Just wow, will take a look this week! Thanks for the huge effort!

cheshire137 · 2017-10-09T04:16:06Z

Just a wee little PR. 😅

lex111 · 2017-10-09T20:09:46Z

@machour I will try to see this PR in detail as soon as possible, but with a cursory examination it is clear that a very cool work! 👏

housseindjirdeh · 2017-10-10T01:46:42Z

Thanks so much for this @machour, this is going to take some time to digest so I'll have some time blocked hopefully in a few days to go through this in detail. 🎉

machour · 2017-10-11T00:36:25Z

Since last comment, I did the following:

Clean up:

made the organization screen rendered immediately by displaying placeholders (need to find a nicer avatar loader)
removed old & unused actions, reducer, types from organization
moved src/organization/screen/* to src/screens/organization/ (proposed re-organisation)

Refactor:

kept only the redux logic in the organization repos screen (container)
created a <RepositoryList /> components that only deals with the UI (dumb).

UI ❤️ :

added a loading indicator at the bottom of the list which disappears when last page is reached

…x-middleware

alejandronanez · 2017-10-13T22:43:48Z

@ all Guys, I'm stepping aside on this PR, I think I don't have enough context right now to make a good decision about what should/should not be done and this is becoming larger (100+ comments + 1.8k lines is already quite difficult to catch up)

machour · 2017-10-14T13:32:58Z

@alejandronanez I totally understand your concern about the size of the PR..
It's a big piece, and it have to be actually pulled and tested step by step by the reviewer.
The initial comments I made were there to answer questions that would may have arise while trying it.
Sorry if it or I got too verbose.

I'm supposed to have a one on one review with @housseindjirdeh tomorrow about this proposal.
I'll do that and see how he feels about it. Maybe things can be split across various PRs and be clearer.
Or maybe my use of redux is unholy, and it should be dismissed all together.

…x-middleware

lex111

An interesting implementation, I've never used Proxy, but this is a good case of using it, and it's not that difficult actually.

lex111 · 2017-10-15T16:14:09Z

+  Alert.alert('API Error', error);
+};
+
+export const withReducers = Provider => {


Can rename to createDispatchProxy?

lex111 · 2017-10-15T16:15:53Z

+  return new Proxy(withReducers, {
+    get: (c, namespace) => {
+      return new Proxy(client[namespace], {
+        get: (endpoint, call) => (...args) => (dispatch, getState) => {


Maybe rename to call -> method?
Or somehow in a different way, because it creates a feeling that this is a function (callback).

Although I like the name action, but it is already in use, although can Actions[actionName] be just the type of action?

const actionType = Actions[actionName]; // and if so?

You're right, that should be "method"

Could you check the last version I just pushed for namings ?

lex111 · 2017-10-15T16:23:43Z

+
+          // Identify if we have our magical last argument
+          const declaredArgsNumber = endpoint[call].length;
+          const isMagicArgAvailable = args.length === declaredArgsNumber;


Magic is not a good name... what if rename to isAdditionalArgsExist or isExtraArgsExists?

I think, can use all the same plural (for the future)?

I really didn't know how to name this extra last argument which should be implemented in all methods definition..
This is an argument that is always at the end, and is used for both internal & external stuff. AdditionalArg doesn't seem to describe it enough

I think it would be easier if we considered that there could be more than one magic parameters, the more it is quite possible, given how it is currently used:

const { loadMore = false } = magicArg; // then maybe `param2`, `param3`... and these are additional parameters

lex111 · 2017-10-15T16:25:38Z

+          const pureArgs = isMagicArgAvailable
+            ? args.slice(0, args.length - 1)
+            : args;
+          const magicArg = isMagicArgAvailable ? args[args.length - 1] : {};


Rename to magicArg -> additionalArg or extraArg.

And can use the plural? magicArg -> additionalArgs or extraArgs.

You can do this: args[args.length - 1] -> args.slice(-1)

lex111 · 2017-10-15T16:27:04Z

+          const isMagicArgAvailable = args.length === declaredArgsNumber;
+
+          const pureArgs = isMagicArgAvailable
+            ? args.slice(0, args.length - 1)


Or can simply args.slice(0, -1)

lex111 · 2017-10-15T16:46:04Z

+    this.authHeaders = { Authorization: `token ${accessToken}` };
+  };
+
+  getNextPageUrl = response => {


Proposal: I think that if pass the Link header instead of the entire response?

This is correct for Github, but for Gitlab for example, next link information is contained in the JSON response. This is why I decided to pass Response

lex111 · 2017-10-15T16:47:39Z

+     * Gets an organization by its id
+     *
+     * @param {string} orgId
+     */


There is no description for params... but I think, but does JSDoc really need it? Usually we do not use it, can remove it, especially when soon will be use Flow?

Description for params will be the same for all methods. I wanted to avoid duplicating doc comments before finding a good name.

The JSDoc is in Github for now, but I'll move it to the Rest class for later. (We're going to add all the blanks prototypes there along with the JSDocs to be able to generate an API documentation like this: https://octokit.github.io/node-github/#api-activity-markNotificationThreadAsRead)

Does that sound ok to you?

lex111 · 2017-10-15T16:56:04Z

+     *
+     * @param {string} userId
+     */
+    getEventsReceived: async (userId, params) => {


Maybe rename to simply getEvents, if the activity is understood as the main screen?

activity is a reflection of GitHub's endpoint, which contains events, notifications, watching, etc : https://developer.github.com/v3/activity/

I'm sticking with node-github naming mostly for now, as it almost reflects the URL called

lex111 · 2017-10-15T16:59:49Z

          underlayColor={colors.greyLight}
        />}
+
+      {!!entity.webSite &&


This is something new? Is this exactly supposed to be here?

The block above should be removed once the new API is implemented on Users.
For now I duplicated the block instead of putting 3 entity.web_site ? entity.web_site : entity.webSite in the existing block

@lex111 left you some more comments on gitter to avoid writing another page here :)

lex111 · 2017-10-15T17:15:16Z

+          if (paginator) {
+            const { loadMore = false } = magicArg;
+            const { pageCount = 0, isFetching = false, nextPageUrl } =
+              paginator[pureArgs.join('-')] || {};


Why need .join('-')?

example:

client.activity.getNotifications(false, false); // unread client.activity.getNotifications(true, false); // participating

We need to paginators here, and to identify them uniquely we use .join('-') to create the key:

Oh, I see, although it's not very clear.

…x-middleware

jouderianjr

Just some comments :D I'm reviewing the code, but overall this is pretty great man 💃 Congratz

jouderianjr · 2017-10-16T13:22:15Z

+      finalUrl = url;
+      // add explicitely specified parameters
+      if (params.per_page) {
+        finalUrl = `${finalUrl}${finalUrl.indexOf('?') !== -1


I think we can use here the method includes = finalUrl.includes('?')

jouderianjr · 2017-10-16T13:22:41Z

+      }
+    }
+
+    if (finalUrl.indexOf(this.API_ROOT) === -1) {


The same, in order do improve the readability, we can use includes

jouderianjr · 2017-10-16T13:25:24Z

+      method,
+      headers: {
+        'Cache-Control': 'no-cache',
+        ...this.authHeaders,


Hey, where the this.authHeaders comes from? 😄

Oh, I just noted that the child class(github/client.js) have this attribute. Maybe should be more clear include a default implementation here? maybe authHeaders = {}

jouderianjr · 2017-10-16T13:33:41Z

+          schema: Schemas.USER_ARRAY,
+        },
+        params
+      ).then(struct => {


This a code style minor thing but you can remove the return key using:

).then(struct => ({ ...struct, nextPageUrl: this.getNextPageUrl(struct.response), }));

jouderianjr · 2017-10-16T16:08:03Z

+  },
+  {
+    idAttribute: event => event.id,
+    processStrategy: entity => {


I think that can be refactor in order to avoid processed. repetition

proccessStrategy: entity => ({ ...initSchema(), id: entity.id, ....

Thanks a lot for your review man! Just went through your remarks and applied the suggestions. I didn't know about String.includes, nice one :)

I just kept EventSchema like that for the moment, cause it still needs some work that may require that processed const. All of this will be polished at the end of the PR.

jouderianjr · 2017-10-16T16:10:35Z

+      if (isInMinimalisticForm(entity)) {
+        processed.id = entity.name.toLowerCase();
+        processed.fullName = entity.name;
+        processed.shortName = entity.name.substring(


It is possible have more than one '/' in the string ?

not for a repo

…x-middleware

machour · 2017-10-18T09:24:14Z

Dears,

After validating this PR principle with @housseindjirdeh & @lex111, and after hearing from @jouderianjr & @alejandronanez I came up with a smoother plan to integrate this PR:

This big and barely readable/reviewable PR is closed as of now
I'll wait for tests to be merged in master (test: begin implementing basic component tests #407)
I'll do a new & smaller PR which includes:
- The api/rest/ folder, which handles all the API/Redux logic
- An internal documentation in markdown format
- Tests for schemas transformations
- The notifications section of the app redone with the new redux approach
- 0 initial comments from my part (😆)

Once this new PR is digested by everyone & merged in, we'll set a plan to migrate the rest of the App.

Thank you all for your constructive feedback!

<3

feat(api): fixlets

1b057fc

machour added discussion refactor 🏗️ work in progress labels Oct 9, 2017

This was referenced Oct 9, 2017

Experimenting with normalizr #350

Closed

wip: use GraphQL API to fetch issues and pull requests #455

Closed

machour added 3 commits October 9, 2017 04:40

Merge branch 'master' into multi-client-redux-middleware

f0948d0

fix(browser): Fix openInBrowser url

0f5746d

fix(build): add lodash

b710e80

machour requested review from Antoine38660, alejandronanez, andrewda, chinesedfan, housseindjirdeh, jouderianjr and lex111 October 9, 2017 03:59

machour mentioned this pull request Oct 9, 2017

Maintain and use a node-github fork? #348

Closed

machour added 7 commits October 10, 2017 20:24

refactor(api): Remove useless files for clarity sake & reducer

b52977a

refactor: group and host screens under src/screens

ae00beb

refactor: group and host screens under src/screens

de4ab42

refactor: get rid of initialOrg :-O

83e92b5

feat: No more waiting screen, mock entity attributes instead

a24f64e

refactor: split repository-list into a dumb component

0ab34f2

feat(list): add load progress indicator, adjust list margin

29fd81f

Merge remote-tracking branch 'upstream/master' into multi-client-redu…

a7cd9e1

…x-middleware

machour added 3 commits October 15, 2017 12:13

Merge remote-tracking branch 'upstream/master' into multi-client-redu…

4703a88

…x-middleware

refactor: add helpers for decorators

da4e6b1

feat: make withCounter() store its computed data in store.counters

8055862

machour mentioned this pull request Oct 15, 2017

fix(locale): make correct locale use #479

Merged

lex111 requested changes Oct 15, 2017

View reviewed changes

machour added 4 commits October 15, 2017 18:26

refactor: switch notifications to the new API

df3515f

fix: Some naming/code clean up using Alexey reviews

1d65a0a

refactor: decorators -> proxies

8c24f81

Merge remote-tracking branch 'upstream/master' into multi-client-redu…

cafa620

…x-middleware

housseindjirdeh mentioned this pull request Oct 16, 2017

Not displaying all PRs or issues #217

Closed

jouderianjr reviewed Oct 16, 2017

View reviewed changes

machour added 10 commits October 16, 2017 18:55

refactor: Move counting logic in the provider. Add abstract methods

9cbabee

refactor: merge createCountProxy into createDispatchProxy

5e576db

Merge remote-tracking branch 'upstream/master' into multi-client-redu…

c12429b

…x-middleware

fix: Locale & Prettier related changes

5c1d82e

refactor: get the proxied client directly from api/rest

10a24ca

style: some code clean up from Jouderian

e2e9ca3

refactor: simplify client.fetch() call

ac036ec

refactor: decorators-helpers => api-helpers. More clean up in proxy

34d6d28

fix: Fix several bugs with schemas, remove dumps

82be98a

test: Implement all schemas tests

40429ca

gitpoint deleted a comment from lex111 Oct 17, 2017

machour added on hold and removed 🏗️ work in progress labels Oct 18, 2017

machour closed this Oct 18, 2017

This was referenced Oct 18, 2017

Allow adding and viewing reviews to PRs #472

Open

Discussion - Move to Redux Actions + Reselect #520

Closed

Conversation

machour commented Oct 9, 2017

The organization profile screen:

Scrolling the members list

In and out of the organization screen

Refreshing the organization screen

Browsing the organization repositories

Moar data

Moar amaze

Schemas are wonderful

Isn't that too much for the store ?

Not persisted

Gitlab? Bitbucket?

Uh oh!

alejandronanez commented Oct 9, 2017

Uh oh!

cheshire137 commented Oct 9, 2017

Uh oh!

lex111 commented Oct 9, 2017

Uh oh!

housseindjirdeh commented Oct 10, 2017

Uh oh!

machour commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

machour commented Oct 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lex111 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!

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!

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!

jouderianjr 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

machour commented Oct 11, 2017 •

edited

Loading

alejandronanez commented Oct 13, 2017 •

edited

Loading

machour commented Oct 14, 2017 •

edited

Loading