refactor(repository-screen): switch to GraphQL #775

machour · 2018-04-17T02:24:55Z

Question	Response
Version?	v1.4.1
Devices tested?	iPhone 7
Bug fix?	no
New feature?	yes
Includes tests?	no
All Tests pass?	no
Related ticket?	#761

Description

Introducing GraphQL in the new Redux API 🎉

Rewrote the repo screen to be loaded with only two requests:

A GraphQL query to load all the displayed information on the screen
A REST list query to load the contributors (not provided by GitHub v4 yet)

I also ported star/unstar watch/unwatch & fork to the new API.

For now, the GraphQL query also returns 50 issues & 50 pull requests, to avoid making any changes to the issues/pulls list screens. We will get rid of that once these two screens are rewritten to the new API.

Even with that little performance annoyance, the new screen is blazing fast!

UX changes

Starring/Unstarring or Watching/Unwatching are immediately reflected on the UI :

watching	unwatching	not watching

This is possible with the new Client.delete() and Client.put() methods, that allow us to patch the entity in our Store. This logic already showcased for REST in #770, is also compatible with GraphQL.

Error Page (to be discussed)

When a repository doesn't exist, we currently display a Toast with an error message, and an incomplete Repository screen.

I wanted to simplify things, a create a nicer user experience, so I tweaked this part a bit.

My first approach was to immediately navigate back & display the toast, but the toast length was really limited.

So for now I'm displaying this error page:

Come to think about it, we already have an ErrorPage component, but with a hardcoded message for now. We could definitely refactor it a bit:

Allow for a custom message to be passed as a prop
Give it some UI ❤️
Allow the user to provide us some feedback about how he got to the error (if it's a GitPoint app error) and have that feedback posted as a new issue in the GitPoint repository ✨

And icing on the cake, we would use that from the new componentDidCatch React lifecycle method and spread that in all our screens.

Any opinions on that ? If accepted, this would have to be done in a separate PR.

NB: For now the new API displays an additional (ugly) alert message meant for debugging.

coveralls · 2018-04-17T02:40:21Z

Coverage decreased (-1.6%) to 41.564% when pulling dc6be2f on machour:refactor/repo-screen into 8f8f229 on gitpoint:master.

housseindjirdeh

So so great that we actually don't have to change much at all when porting to GraphQL :) It's literally a matter of adding the queries and mapping them correct?

I love this and everything looks great, feel free to merge whenever ✅

Regarding the error screen - I agree that it might be nicer to have one instead of a toast. But I also agree that the current implementation is a little much, and we can definitely come to a better decision to have a better ErrorComponent or ErrorScreen that is used everywhere and is a little more flexible. When we do that, we can apply some better UI changes. For now, this is not a problem :)

housseindjirdeh · 2018-04-17T20:47:42Z

src/api/client.js

+          id: repoId,
+          updater: gqlRepo => ({
+            isSubscribed: true,
+            watchersCount: gqlRepo.watchersCount + 1,


Good stuff :)

this was almost already in #770, but changed it to take a callback with the modified elements, for way more flexibility.

housseindjirdeh · 2018-04-17T20:49:53Z

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

+                }
+
+                return Promise.resolve();
+              }


No worries about doing it now, we can chat about this much later - but because this single method is getting so long we can spend a little time separating these to separate methods

This is something that have been puzzling me for a while, but I can't seem to find a good code splitting approach, without passing a zillion of parameters to the separate methods.

Would love if you can lay a fresh pair of eyes on this 🙌

housseindjirdeh · 2018-04-17T20:50:52Z

src/api/queries/repo.query.js

+  }
+}
+
+`;


👏 👏 👏 👏

housseindjirdeh · 2018-04-17T20:51:40Z

src/api/reducers/entities.js

@@ -7,6 +7,7 @@ export const entities = (
    orgs: {},
    repos: {},
    users: {},
+    gqlRepos: {},


Why are we separating it into it's own entity instead of just using repos? I'm assuming this is because other parts of the app still reference the original repos object (and if that's the case -> not a big deal for now)

repos here is from our new store architecture. It holds v3 repositories found in the Search screen, or starred by a user for example.

I needed to separate repositories retrieved via GraphQL , from the one retrieved from the Rest API, because they don't share the same structure / field names.

If you get back to #761, you'll see that I've mentioned adapting GraphQL entities structure to match the ones from Rest.

But now that I've experimented a bit with GraphQL, I think that we should be doing exactly the opposite: Adapt REST entities to the GraphQL structure in our store. We will do that once we migrated everything to the new API, because it will require updates in most of our components (avatar_url => avatarUrl).

Furthermore, since we're retrieving exactly what we need from GraphQL, I think that we should be storing the response structure as-is in the store. I'll be updating this PR accordingly to reflect that (and loose the ugly code in gqlRepos schema)

housseindjirdeh · 2018-04-17T20:55:16Z

src/repository/repository.action.js

-      });
-  };
-};
-


machour · 2018-04-18T15:33:02Z

src/api/schemas/gql-repos.js

+        ),
+        contents_url: `https://api.github.com/repos/${
+          repository.nameWithOwner
+        }/contents`,


This is the file I just called ugly. We're receiving a structure that correspond exactly to the current GitHub data model & relation, and I'm just breaking everything to try to mimic the old Rest structure. Sounds totally illogic.

machour · 2018-04-18T15:36:11Z

Thanks for the feedback buddy. As mentioned in the comments, I think I'll be tweaking the data structure a bit more before merging this one.

Currently experimenting with GraphQL on the Issue/Pull Request screen to gain more hindsight about how we should store the data from v4.

machour · 2018-04-22T13:44:56Z

Reverted to a nicer error screen (the one that was put in place by #726).

Also, as mentioned in the conversation, I've changed the structure of our data in store to closely match the data structure of GitHub & the new Repository screen accept only a repoId as entry parameter.

machour · 2018-04-24T16:00:07Z

@housseindjirdeh merged this PR as I had your okay. Changes since your comment only enhanced it.

machour added 4 commits April 16, 2018 23:57

wip: implement graphql on the repo screen

91c9916

refactor: migrate watch/unwatch/star/unstar/fork

d126955

refactor: handle errors

6d74350

fix: put back these methods, still used by Issue screen

2361149

machour added ready for review discussion graphql labels Apr 17, 2018

fix: update tests

662ed9b

housseindjirdeh approved these changes Apr 17, 2018

View reviewed changes

machour commented Apr 18, 2018

View reviewed changes

machour added 3 commits April 22, 2018 11:37

fix: revert to a nicer error screen

90263c4

refactor: stick to GitHub data structure in our store

385316b

refactor: more clean up

dc6be2f

machour merged commit 9eeb5a9 into gitpoint:master Apr 24, 2018

machour deleted the refactor/repo-screen branch April 24, 2018 16:00

machour mentioned this pull request Dec 1, 2018

Rest: Migrate paginated calls to the new API #751

Open

13 tasks

chinesedfan mentioned this pull request Dec 2, 2018

feat: Support pagination in issue-list screen and pull-list screen #856

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(repository-screen): switch to GraphQL #775

refactor(repository-screen): switch to GraphQL #775

Uh oh!

machour commented Apr 17, 2018

Uh oh!

coveralls commented Apr 17, 2018 •

edited

Loading

Uh oh!

housseindjirdeh left a comment

Uh oh!

housseindjirdeh Apr 17, 2018

Uh oh!

machour Apr 18, 2018

Uh oh!

housseindjirdeh Apr 17, 2018

Uh oh!

machour Apr 18, 2018

Uh oh!

housseindjirdeh Apr 17, 2018

Uh oh!

housseindjirdeh Apr 17, 2018

Uh oh!

machour Apr 18, 2018

Uh oh!

housseindjirdeh Apr 17, 2018

Uh oh!

machour Apr 18, 2018

Uh oh!

machour commented Apr 18, 2018

Uh oh!

machour commented Apr 22, 2018

Uh oh!

machour commented Apr 24, 2018

Uh oh!

Uh oh!

+                }
+              }
+              `;

refactor(repository-screen): switch to GraphQL #775

refactor(repository-screen): switch to GraphQL #775

Uh oh!

Conversation

machour commented Apr 17, 2018

Description

UX changes

Uh oh!

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

Uh oh!

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!

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 commented Apr 18, 2018

Uh oh!

machour commented Apr 22, 2018

Uh oh!

machour commented Apr 24, 2018

Uh oh!

Uh oh!

coveralls commented Apr 17, 2018 •

edited

Loading