feat: Paged events screen #736

machour · 2018-02-23T23:02:59Z

Hi @gitpoint/maintainers,

This is my third attempt of the redux refactoring, disguised as API pagination for the events screen 😄

Having learned a great deal from the previous PR, I'll try to make the smallest set of changes possible and stay focused on a first task, while setting the path for a great deal of internal changes.

This PR implements paginated API calls for the events screen. It does so by implementing a brand new v3 API Client, and a "dispatch proxy" wrapped around it.

Dispatch proxy ??!!

You can imagine the dispatch proxy as a single point of entry, that helps automating a lot of stuff.

Before this PR, the Events Screen used to call getUserEvents. getUserEvents dispatched the PENDING action, and proceeded to call fetchUserEvents which made the API call.
The response was then reduced manually in the corresponding switch case in auth.reducer.js and either ERROR or SUCCESS were dispatched.

This is not really different with the dispatch proxy. You map the relevant Client method (client.activity.getEventsReceived('username') in this case) to props, and when it gets called, here's what happens automatically:

the Proxy receives the actual call and analyzes it
it retrieve the relevant action set (to be used when dispatching)
it sets the accessToken for the next client call
it dispatches PENDING
it performs the actual API call (calling Client), and analyzes the response:
- if it's an error, it shows an error message in an Alert Box and dispatches ERROR
- if it's a success, it analyzes the JSON and put data into our store, then dispatches SUCCESS

Nothing fantastic yet.. let's get a closer look to the data

data in the Store

Our current store design is flawed, since we can't for example have two different issues in two different tabs without things getting messy (there's only one issue slot).

I introduced normalizr to help us with that. It's an amazing mapping tool that takes a JSON + a schema definition, and reorganizes the JSON into an entities dictionary. I used it to store the received events by their ids in store:

So, so far the 30 received events from GitHub API got reduced in getStore().entities.events, indexed by their ids.

What was also created in store, is the pagination data, which was extracted from the Link response header:

In getStore().pagination.ACTIVITY_GET_EVENTS_RECEIVED.machour we have the pagination data associated with the call to client.activity.getEventsReceived('machour').

This pagination data consist of:

isFetching: true if currently fetching data from the api, false otherwise
nextPageUrl: the URL that the Client needs to call to get the next page of result
pageCount: the number of pages fetched so far
ids: an array of events ids.

As I fetched the first page of results, I have 30 ids, and every id can be found under getStore().entities.events

If I fetch the next page of results (by scrolling to the bottom of the list), here's what I get in my pagination after the call succeeds:

nextPageUrl is how ready for the third call
I have 30 more ids, which got prepended to my list

when did all that happen?!

remember that it was the Proxy who triggered the call and handled the response? that was an understatement:

For the first page of result, the method called from the Screen was client.activity.getReceivedEvents(machour).
The Client called this API url: https://api.github.com/users/machour/received_events
and returned the JSON response from GitHub + two additional things:

The normalizr schema to be used (this is how we instructed normalizr to store the events by their ids)
The nextPageUrl

For the second page of result, when reaching the end of the FlatList, the method called from the Screen was client.activity.getReceivedEvents(machour, { loadMore: true })

having intercepted that call, the proxy injected the nextPageUrl in the Client call which made him call https://api.github.com/user/304450/received_events?page=2 and return the second set of data.

isn't that like too much for a simple pagination?

This is a first PR aimed at making the less changes possible in the User/Screen land while implementing a solid internal engine. If you look at the actual code used to implement events retrieval and pagination, it's as simple as this:

The method in Client:

    getEventsReceived: async (userId, params) => {
      return this.fetch(`users/${userId}/received_events`, params).then(
        response => ({
          response,
          nextPageUrl: this.getNextPageUrl(response),
          schema: Schemas.EVENT_ARRAY,
        })
      );
    },

The declaration of the set of Actions:

export const ACTIVITY_GET_EVENTS_RECEIVED = createPaginationActionSet(
  'ACTIVITY_GET_EVENTS_RECEIVED'
);

Enabling the reducer:

export const pagination = combineReducers({
  // the following line:
  ACTIVITY_GET_EVENTS_RECEIVED: paginate(Actions.ACTIVITY_GET_EVENTS_RECEIVED),
});

This is almost ALL the code needed to implement a paginated API call with this new engine (give or take the schema creation) and I believe that having a such simple high-end API will enable us to implement more features in GitPoint, way quicker.

The proxy code is as complicated as it will get (or almost) and I will be adding more helpers to ease things on the Screen side.

Frankly .. I didn't get it

This is absolutely normal as it is quiet lot to take in, and I might have skipped some explanations.
I'll be more than happy to further explain things on Gitter 🙌

coveralls · 2018-02-23T23:11:17Z

Coverage decreased (-3.6%) to 43.386% when pulling 73b6609 on machour:paged-events-screen into dd2a50c on gitpoint:master.

housseindjirdeh · 2018-02-23T23:49:01Z

This is great, thank you so so much @machour

I'm going to dive in during the weekend 🙏 I'm definitely going to ping you with some questions 😅

chinesedfan

@machour Shared some personal ideas with you. I can't wait to see this great improvement got merged. It implements nearly the same feature with less and simpler codes.

chinesedfan · 2018-02-24T13:49:28Z

src/api/proxies/create-dispatch-proxy.js

+export const createDispatchProxy = Provider => {
+  const client = new Provider();
+
+  return new Proxy(createDispatchProxy, {


If only need to intercept get, is a simple wrapper enough?

how would you code that? I need to intercept any getter called.
in this first proxy, this is the API namespace ("activity", "repository", "...") : client.**activity**.something()

I am not sure. I just wonder it should be able to work. Maybe we should separate Client as utils and api config first.

Will take a little bit of time for me trying to digest exactly how this proxy works 😅 , I'm going to step through it again tomorrow 🙏

The idea behind this proxy - I am all for it without a DOUBT. It can make all of our lives a lot easier

Keep in mind that the first Proxy is just for cosmetics. It enables us to have namespaces.
i.e. client.activity.getEventsReceived() instead of client.activityGetEventsReceived()

chinesedfan · 2018-02-24T13:51:00Z

src/api/proxies/create-dispatch-proxy.js

+                return Promise.resolve();
+              }
+
+              return struct.response.json().then(json => {


If possible, I prefer to keep only 1 level then, instead of nested.

we can definitely do that. Keeping it nested for now to avoid more changes // complicated review process

After a second look, the current implementation seems clearer to me.
There are two levels all in all, with the second one not being used if an error happened in the first one.

Feel free to propose the change when this is merged tho!

chinesedfan · 2018-02-24T13:55:46Z

src/api/reducers/pagination.js

+
+// Updates the pagination data for different actions.
+export const pagination = combineReducers({
+  ACTIVITY_GET_EVENTS_RECEIVED: paginate(Actions.ACTIVITY_GET_EVENTS_RECEIVED),


How about saving actions in an independent file, and generating this reducer automatically?

import actions from 'xx/actions'; export const pagination = combineReducers(actions.map(paginate));

Neat! That would mean no need to edit pagination.js when adding something new 👌

I tried and failed miserably 😅 Since this is a cosmetic thing, let's enhance it later, when we have more actions (some of them won't be paginated)

chinesedfan · 2018-02-24T13:59:38Z

src/auth/screens/events.screen.js

@@ -496,17 +503,43 @@ class Events extends Component {
    );
  }

+  renderFooter = () => {


If every list shares the same loading, we can extract it as a component.

YES! This would be done in a subsequent PR and we'll have an issue to track FlatList to ApiPaginatedList migrations, like we did for styled-components

Agreed, that would be great to abstract away in a separate component 🚀

chinesedfan · 2018-02-24T14:02:09Z

src/api/client.js

+  fetch = async (
+    url,
+    params = {},
+    { method = this.Method.GET, body = {}, headers = {} } = {}


Can we keep the interface the same with the original fetch(url, options)?

I guess I can move params inside of the third parameter:

fetch(url, { method = this.Method.GET, body = {}, headers = {}, params = {} } = {})

alejandronanez · 2018-02-26T01:28:25Z

src/api/client.js

+    return response
+      .json()
+      .then(data => data.length)
+      .catch(() => 0);


Hey @machour! question here, what is () => 0 purpose? I guess you're handling errors in a subsequent PR maybe?

(this method isn't used yet, and is a leftover from my previous PR, sorry for that)
Yes, we will add proper error handling if the json parsing failed when we start using it.

Maybe we can leave a TODO there (or have a simple console.log) for now

machour · 2018-03-07T16:48:15Z

👋 @housseindjirdeh, let's move forward with this as soon as possible for you mate.

housseindjirdeh · 2018-03-01T02:59:49Z

package.json

@@ -57,10 +57,12 @@
    "enzyme": "^3.0.0",
    "enzyme-adapter-react-16": "^1.0.0",
    "fuzzysort": "^1.0.1",
+    "lodash": "^4.17.5",


If we're including all of lodash, can we remove the uniqby specific package?

housseindjirdeh · 2018-03-01T03:00:47Z

src/api/client.js

+    return response
+      .json()
+      .then(data => data.length)
+      .catch(() => 0);


Maybe we can leave a TODO there (or have a simple console.log) for now

housseindjirdeh · 2018-03-01T03:04:51Z

src/api/client.js

+      .trim()
+      .split(';')[0]
+      .slice(1, -1);
+  };


Maybe we can simplify this method to something like:

getNextPageUrl = response => { const { headers } = response; const link = headers.get('link'); const nextLink = link ? link.split(',').find(s => s.indexOf('rel="next"') > -1) : null; if (!nextLink) { return; } return nextLink .trim() .split(';')[0] .slice(1, -1); };

Not picky however 😁 The way it currently is is also fine

housseindjirdeh · 2018-03-01T03:07:43Z

src/api/proxies/create-dispatch-proxy.js

+export const createDispatchProxy = Provider => {
+  const client = new Provider();
+
+  return new Proxy(createDispatchProxy, {


Will take a little bit of time for me trying to digest exactly how this proxy works 😅 , I'm going to step through it again tomorrow 🙏

The idea behind this proxy - I am all for it without a DOUBT. It can make all of our lives a lot easier

housseindjirdeh · 2018-03-01T03:08:37Z

src/auth/screens/events.screen.js

@@ -496,17 +503,43 @@ class Events extends Component {
    );
  }

+  renderFooter = () => {


Agreed, that would be great to abstract away in a separate component 🚀

housseindjirdeh

This is great mate, and I'm happy to merge this in if you think it's good to start having in the app now. I'll be spending the rest of this week diving deeper into the proxy logic but I trust your judgment for the small parts I haven't looked too closely yet - especially since you mentioned it shouldn't be getting any more complex than the way it is really :)

Thanks again for everything fam, this can simplify things so much at the top level it's not even funny

machour

Went through the feedback and acted accordingly.

Decided to keep some of the enhancements proposed by @chinesedfan for later as I wasn't able to come with something satisfying, and they are not blocking.

By the way @chinesedfan, matching fetch() wasn't possible (cause it would nuke the default values). I renamed client.fetch() to client.call() to avoid confusions.

@housseindjirdeh all yours to merge fam!

machour · 2018-03-22T09:21:06Z

src/api/client.js

+      .trim()
+      .split(';')[0]
+      .slice(1, -1);
+  };


machour · 2018-03-22T09:23:08Z

src/api/proxies/create-dispatch-proxy.js

+export const createDispatchProxy = Provider => {
+  const client = new Provider();
+
+  return new Proxy(createDispatchProxy, {


Keep in mind that the first Proxy is just for cosmetics. It enables us to have namespaces.
i.e. client.activity.getEventsReceived() instead of client.activityGetEventsReceived()

machour · 2018-03-22T09:25:57Z

src/api/proxies/create-dispatch-proxy.js

+                return Promise.resolve();
+              }
+
+              return struct.response.json().then(json => {


After a second look, the current implementation seems clearer to me.
There are two levels all in all, with the second one not being used if an error happened in the first one.

Feel free to propose the change when this is merged tho!

machour · 2018-03-22T09:28:28Z

src/api/reducers/pagination.js

+
+// Updates the pagination data for different actions.
+export const pagination = combineReducers({
+  ACTIVITY_GET_EVENTS_RECEIVED: paginate(Actions.ACTIVITY_GET_EVENTS_RECEIVED),


I tried and failed miserably 😅 Since this is a cosmetic thing, let's enhance it later, when we have more actions (some of them won't be paginated)

chinesedfan · 2018-03-23T14:06:30Z

@machour Agreed! Let's move forward first!

machour added 2 commits February 23, 2018 21:34

feat: implement pagination in events screen

b204895

refactor: remove old code that used to handle this

282aa3d

machour requested review from housseindjirdeh and chinesedfan February 23, 2018 23:03

machour changed the title ~~Paged events screen~~ feat: Paged events screen Feb 23, 2018

machour added refactor ready for review labels Feb 23, 2018

chinesedfan requested changes Feb 25, 2018

View reviewed changes

alejandronanez reviewed Feb 26, 2018

View reviewed changes

housseindjirdeh reviewed Mar 22, 2018

View reviewed changes

housseindjirdeh approved these changes Mar 22, 2018

View reviewed changes

machour added 3 commits March 22, 2018 09:08

chore: get rid of lodash.uniqby, already included in lodash

504fb9d

refactor: simplify code and add TODO item

7a484ab

refactor: rename Client.fetch() to Client.call() to avoid confusions

42c5d6f

machour commented Mar 22, 2018

View reviewed changes

chinesedfan approved these changes Mar 23, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into paged-events-screen

73b6609

machour merged commit 7f82b48 into gitpoint:master Mar 24, 2018

machour deleted the paged-events-screen branch March 24, 2018 13:43

machour mentioned this pull request Mar 26, 2018

Rest: Migrate paginated calls to the new API #751

Open

13 tasks

feat: Paged events screen #736

feat: Paged events screen #736

Uh oh!

Conversation

machour commented Feb 23, 2018

Uh oh!

coveralls commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

housseindjirdeh commented Feb 23, 2018

Uh oh!

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

housseindjirdeh Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

machour Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

machour commented Mar 7, 2018

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 Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

machour left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 23, 2018 •

edited

Loading

housseindjirdeh Mar 1, 2018 •

edited

Loading

machour Feb 26, 2018 •

edited

Loading

housseindjirdeh Mar 1, 2018 •

edited

Loading