wip: use GraphQL API to fetch issues and pull requests by cheshire137 · Pull Request #455 · gitpoint/git-point

cheshire137 · 2017-10-08T21:55:05Z

This is a work in progress to solicit some feedback about the approach. If this seems worthwhile, I'll continue working on this branch!

This branch partially addresses #141 by updating how a repository's issues and pull requests are fetched, so we use v4 of the GitHub API instead of v3. What's missing currently is the views for individual issues and pull requests are not working because they're still expecting data in the v3 format instead of v4. I added a new getPullRequests function because PRs are not automatically returned in the list of issues in v4 of the API.

There are a couple style changes I didn't intentionally make. 🤔 I believe when I commit my changes, a git hook is doing some automatic style fixes and it's adjusting lines I didn't otherwise touch. Specifically, it's removing extra parentheses from around components. Is this expected?

This branch also adds me to the contributors list. 🎉

Since project link was removed, this is the new reference to the preferred issues to work on.

I ran `yarn contributors:generate` after I added myself to .all-contributorsrc.

lex111 · 2017-10-09T00:16:37Z

Well done, it's time to GraphQL!

@cheshire137 there is another approach to do GraphQL queries, we already discussed this in PR #347: you pass currently a ready query (with values variables), but instead you can only pass the query and variables to it. We considered this way, because GraphQL queries may be large and we wanted to store them in separate files, but by now we have not decided how and where to store them. @machour @housseindjirdeh is not it?

housseindjirdeh · 2017-10-09T00:38:25Z

This looks solid!! 💃

I'm still relatively new to GraphQL and I'm not sure @lex111. It does look like queries can be quite large so do you think we should keep them in the actions file like we have here or in the api file similar to your PR branch? Let us know what you think @cheshire137. Also, @machour will appreciate your thoughts on this as well.

And with regards to formatting changes, you're right :) We're using prettier as a pre-commit hook hence why you're noticing a few things different.

machour · 2017-10-09T03:28:30Z

Wow wow, that's looking just amazing @cheshire137 !
I'll give your PR a try as soon as possible and see how it plays out.

I've been busy on my side with another PR that I just posted: #457.

It's a big refactoring of our current store, and it introduces a redux middleware and a completely new REST API that is aiming at replacing v3. ( 😄 )

Could you give it a read on your side, and see how this new store could be used for graphql?

cheshire137 · 2017-10-09T04:14:50Z

@lex111, I like what you're doing with the variable declarations in #347, like query($login: String!).

It does look like queries can be quite large so do you think we should keep them in the actions file like we have here or in the api file similar to your PR branch?

I agree GraphQL queries can get long, especially if they stay readable by being broken onto multiple lines. I like the idea of keeping multiple queries in a file, but only the ones related to each other. So not one giant file of all the GraphQL queries used in the whole app. I think src/api/queries/repositories.js or src/repository/repository.queries.js makes sense in this branch. I'll pick one style but I'm happy to change.

I'll give your PR a try as soon as possible and see how it plays out.

Thanks! Beware right now when you click an issue or PR from a repo page, the issue/PR page will be empty of data.

Could you give it a read on your side, and see how this new store could be used for graphql?

Yes! I'll look.

machour

I tested & reviewed your PR @cheshire137, and it's coming out nicely.

I left a few comments here and there with some advices / questions.

I know this is still WIP, but the issues/pulls screens seems to be empty when I visit them. Is that intended for now or did I hit a bug?

Thanks again for all this, I couldn't think of a better time for implementing GraphQL!

machour · 2017-10-09T12:00:29Z

+Please feel free to take on any issue that's currently open. You could look at
+[issues labeled "high priority"](https://github.com/gitpoint/git-point/issues?q=is%3Aopen+is%3Aissue+label%3A%22high+priority%22),
+but feel free to resolve any issue that you would enjoy working on even if it happens to be a low priority.



not sure this should be a part of this PR

Fair enough, personally don't mind having this correction included however.

machour · 2017-10-09T12:01:54Z

+
+    if (response.status >= 200 && response.status < 300) {
+      return json.data;
+    }


Could we test for failed response.statuses instead before doing the JSON.parse() ?

The GraphQL API returns JSON on error as well as success. I do the JSON parsing ahead of this line so I can get the message out of the body as to why the query failed, to put in the Error.

Didn't know that, thanks for clarifying it, it's perfect that way then.

I might be able to do await response.json() instead of parsing the body myself, though. 🤔

And it makes more sense along with Promise.reject() 👌

machour · 2017-10-09T12:03:07Z

+
+    error.response = response;
+    throw error;
+  },


What about return Promise.reject(error.response) instead?

machour · 2017-10-09T12:04:29Z

+      method: METHOD.POST,
+      headers: {
+        Authorization: `token ${accessToken}`,
+      },


Haven't tried GraphQL on my side yet. Are this header enough? Shouldn't we be passing a User-Agent ? (GitPoint/Android-3.2 for example).

I'd like to have it shared between GraphQL & Rest

This is all that's necessary to use the endpoint, though I could add a user-agent header. See the example at https://developer.github.com/v4/guides/forming-calls/#communicating-with-graphql.

For trying the GraphQL API, I like https://developer.github.com/v4/explorer/, which lets you run queries as your GitHub user in the browser.

machour · 2017-10-09T12:05:13Z

  if (type === 'issue') {
-    return issue.state === 'closed' ? 'issue-closed' : 'issue-opened';
+    return issue.state === 'CLOSED' ? 'issue-closed' : 'issue-opened';
  }


CLOSED, OPEN, .. should be moved in a string enum

machour · 2017-10-09T12:07:03Z

+    const response = await v4.call(v4.parameters(accessToken, body));
+
+    return response;
+  },


is post() the only method that will exist to access GraphQL ?
If that's the case, maybe we can merge call() & parameters() inside it

Yep, all requests whether they're reading or writing, are POSTs; see https://developer.github.com/v4/guides/forming-calls/#communicating-with-graphql.

I can merge these methods!

machour · 2017-10-09T12:10:25Z

+    } else if (params.issue) {
+      issueRepository = params.issue.repository.nameWithOwner;
+    }
+    const repositoryUrl = `https://api.github.com/repos/${issueRepository}`;


Should use v3.root

machour · 2017-10-09T12:14:53Z

-    const openIssues = pureIssues.filter(issue => issue.state === 'open');
+    const openPulls = pullRequests.filter(pull => pull.state === 'OPEN');
+    const openIssues = issues.filter(issue => issue.state === 'OPEN');
    const showFork =


Using reactotron, I see that when we navigate to a repository screen, we pull 100 issues & 100 pull requests from the repo.
If the user doesn't click on "View All" for issues or PR, this is a waste of data & Github credit.

Could we have a single GraphQL query that retrieves only the 3 most recent & open issues & PR instead?

Yes! I wondered about this, but was trying to translate directly from how the v3 API was being used to v4. I'll change the code that's slicing the first three results in JavaScript so that instead we just pass first: 3 to the API.

cheshire137 · 2017-10-09T15:16:08Z

the issues/pulls screens seems to be empty when I visit them. Is that intended for now or did I hit a bug?

That's intended! It's where I left off.

machour · 2017-10-09T15:36:40Z

Thanks for the feedback @cheshire137.

I remembered that @andrewda did an extensive study on GraphQL ressources limitation, and the queries scores in #141, and I wonder in what place should we use GraphQL, and where we should instead rely on REST to stay under the limits.

The way I see it, GraphQL is great for screens built with various data from different endpoint. The repository screen is a good example, here are the REST requests currently made:

get the repository information
get the repository forks count
get the contributors
check the existence of the README.md file
get 100 issues
get 100 pull requests

Do you think these six REST calls could be reduced to a single GraphQL query, that would return:

repo basic informations (the one displayed on that screen)
the 30 first contributors, with their login & avatarUrl
a boolean that tells if the README file exists
the 3 most recent issues with status OPEN
the 3 most recent pulls with status OPEN
the total number of open issues
the total number of open pulls
the total number number of forks

On the other hand, I'm not sure GraphQL would be preferable to REST if we just want to paginate the issues list for example, as we're able to get 100 issues in a single REST call, without consuming any credit available for GraphQL calls.

I'm curious about your thoughts on that

cheshire137 · 2017-10-09T15:42:40Z

the 30 first contributors, with their login & avatarUrl

To my knowledge, there's not a connection in GraphQL that returns a repository's contributors. How were you getting at this list via the REST API?

The other data in your list--repo basic info, if the README exists, etc.--all sound fine to fetch via GraphQL.

I'm not sure GraphQL would be preferable to REST if we just want to paginate the issues list for example, as we're able to get 100 issues in a single REST call, without consuming any credit

This is something to consider. The biggest reason to use GraphQL to me is to get at the data we want more directly, and without getting a bunch of other data we don't need.

cheshire137 · 2017-10-09T15:48:30Z

...without consuming any credit available for GraphQL calls.

Can you share a link with info about GraphQL credits?

machour · 2017-10-09T15:50:42Z

Contributors support seems to have been just added a few days ago: https://platform.github.community/t/cant-access-the-contributors-of-a-repository/1848/8?u=machour

With rest, we are using this endpoint https://api.github.com/repos/gitpoint/git-point/contributors

Agreed with you, being able to hand pick the returned fields is gold with GraphQL. Weird REST doesn't provide the same thing via a ?fields GET parameters (gitlab & bitbucket do it afair)

Here's for GraphQL scoring system at GitHub: https://developer.github.com/v4/guides/resource-limitations/ (haven't done any math yet, so I'm maybe worrying for nothing)

cheshire137 · 2017-10-09T15:53:20Z

Looks like 100 issues in GraphQL should just cost 1, and that that rate limit should be shared with the REST API. So using 1 credit for GraphQL means there's 1 fewer for REST. It starts to jump if we nest connections (like grab 100 repos and for each of those, grab 100 issues).

We can also stick rateLimit { cost } in a GraphQL query to see how much the query will cost. We can do rateLimit(dryRun: true) { cost } to see the cost of a query without actually having it count or getting our requested data back.

cheshire137 · 2017-10-09T15:54:27Z

Contributors support seems to have been just added a few days ago

Oh nice! Then your whole list can be fetched via GraphQL if we want.

housseindjirdeh · 2017-10-15T00:01:18Z

      ]
+    },
+    {
+      "login": "cheshire137",


G2Jose · 2017-10-24T00:52:55Z

Have you guys also looked into Relay and Apollo ?

cheshire137 · 2017-11-05T17:10:05Z

Time got away with me on this one! If someone else wanted to take this over from me, I'd be happy to let them.

machour · 2018-04-23T10:12:21Z

Closing this PR as it's superseded by the ongoing store refactoring. Thank you so much for all the efforts and explanations @cheshire137, the next implementation is inspired by your work!

cheshire137 added 5 commits October 8, 2017 16:33

docs: Link to high priority issues

ea5b1b7

Since project link was removed, this is the new reference to the preferred issues to work on.

docs: Add myself as a contributor

be68d51

I ran `yarn contributors:generate` after I added myself to .all-contributorsrc.

feat: Add v4 API helper for using GraphQL

4a0fc5f

feat: Fetch repo issues list via GraphQL API

4e5012a

feat: Get pull requests from GraphQL

1f638b8

andrewda changed the title ~~WIP - Use GraphQL API to fetch issues and pull requests~~ wip: use GraphQL API to fetch issues and pull requests Oct 8, 2017

andrewda added the 🏗️ work in progress label Oct 8, 2017

cheshire137 added 2 commits October 8, 2017 23:24

refactor: Pull issue and PR queries into separate file

302b6d4

refactor: Don't hard-code vars in queries

f40d29a

machour requested changes Oct 9, 2017

View reviewed changes

cheshire137 added 5 commits October 14, 2017 15:23

refactor: Queries can just be strings, not funcs

c71eaeb

refactor: Pull API URL from v3.root

afd2796

fix: Pull issue/PR type from given type prop

d139ac6

fix: Expect GraphQL createdAt property

db3328e

fix: Determine v3 URL for issue comments

0e3a8fa

housseindjirdeh reviewed Oct 15, 2017

View reviewed changes

Comment thread .all-contributorsrc

]

},

{

"login": "cheshire137",

Copy link
Copy Markdown

Member

housseindjirdeh Oct 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

machour mentioned this pull request Oct 24, 2017

Discussion - Move to Redux Actions + Reselect #520

Closed

machour closed this Apr 23, 2018

chinesedfan added the graphql label Oct 30, 2018

Conversation

cheshire137 commented Oct 8, 2017

Uh oh!

lex111 commented Oct 9, 2017

Uh oh!

housseindjirdeh commented Oct 9, 2017

Uh oh!

machour commented Oct 9, 2017

Uh oh!

cheshire137 commented Oct 9, 2017

Uh oh!

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

cheshire137 Oct 9, 2017 • 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!

cheshire137 commented Oct 9, 2017

Uh oh!

machour commented Oct 9, 2017

Uh oh!

cheshire137 commented Oct 9, 2017

Uh oh!

cheshire137 commented Oct 9, 2017

Uh oh!

machour commented Oct 9, 2017

Uh oh!

cheshire137 commented Oct 9, 2017

Uh oh!

cheshire137 commented Oct 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G2Jose commented Oct 24, 2017

Uh oh!

cheshire137 commented Nov 5, 2017

Uh oh!

machour commented Apr 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cheshire137 Oct 9, 2017 •

edited

Loading