-
Notifications
You must be signed in to change notification settings - Fork 775
wip: use GraphQL API to fetch issues and pull requests #455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ea5b1b7
be68d51
4a0fc5f
4e5012a
1f638b8
302b6d4
f40d29a
c71eaeb
afd2796
d139ac6
db3328e
0e3a8fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,9 @@ If you want to open a PR that fixes a bug or adds a feature, then we can't thank | |
|
|
||
| ### Working on issues | ||
|
|
||
| Please feel free to take on any issue that's currently open. Feel free to resolve any issue that you would enjoy working on even if it happens to be a low priority. | ||
| 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. | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure this should be a part of this PR
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough, personally don't mind having this correction included however. |
||
| ## Setup | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,36 @@ const METHOD = { | |
| POST: 'POST', | ||
| }; | ||
|
|
||
| export const v4 = { | ||
| root: 'https://api.github.com/graphql', | ||
| call: async parameters => { | ||
| const response = await fetch(v4.root, parameters); | ||
| const json = JSON.parse(response._bodyText); | ||
|
|
||
| if (response.status >= 200 && response.status < 300) { | ||
| return json.data; | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we test for failed response.statuses instead before doing the JSON.parse() ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't know that, thanks for clarifying it, it's perfect that way then.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might be able to do
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And it makes more sense along with Promise.reject() 👌 |
||
| const error = new Error(json.message); | ||
|
|
||
| error.response = response; | ||
| throw error; | ||
| }, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about |
||
| parameters: (accessToken, body = {}) => { | ||
| return { | ||
| method: METHOD.POST, | ||
| headers: { | ||
| Authorization: `token ${accessToken}`, | ||
| }, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| body: JSON.stringify(body), | ||
| }; | ||
| }, | ||
| post: async (accessToken, body) => { | ||
| const response = await v4.call(v4.parameters(accessToken, body)); | ||
|
|
||
| return response; | ||
| }, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||
| }; | ||
|
|
||
| export const v3 = { | ||
| root: 'https://api.github.com', | ||
| call: async (url, parameters) => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,55 +55,58 @@ const styles = StyleSheet.create({ | |
|
|
||
| const getIconName = (type, issue) => { | ||
| if (type === 'issue') { | ||
| return issue.state === 'closed' ? 'issue-closed' : 'issue-opened'; | ||
| return issue.state === 'CLOSED' ? 'issue-closed' : 'issue-opened'; | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| return 'git-pull-request'; | ||
| }; | ||
|
|
||
| export const IssueListItem = ({ type, issue, navigation, language }: Props) => | ||
| <TouchableHighlight | ||
| style={issue.state === 'closed' && styles.closedIssue} | ||
| onPress={() => | ||
| navigation.navigate('Issue', { | ||
| issue, | ||
| isPR: !!issue.pull_request, | ||
| language, | ||
| })} | ||
| underlayColor={colors.greyLight} | ||
| > | ||
| <View style={styles.container}> | ||
| <ListItem | ||
| containerStyle={styles.listItemContainer} | ||
| title={issue.title} | ||
| subtitle={ | ||
| issue.state === 'open' | ||
| ? translate('issue.main.openIssueSubTitle', language, { | ||
| number: issue.number, | ||
| user: issue.user.login, | ||
| time: moment(issue.created_at).fromNow(), | ||
| }) | ||
| : translate('issue.main.closedIssueSubTitle', language, { | ||
| number: issue.number, | ||
| user: issue.user.login, | ||
| time: moment(issue.closed_at).fromNow(), | ||
| }) | ||
| } | ||
| leftIcon={{ | ||
| name: getIconName(type, issue), | ||
| size: 36, | ||
| color: issue.state === 'open' ? colors.green : colors.red, | ||
| type: 'octicon', | ||
| }} | ||
| hideChevron | ||
| titleStyle={styles.title} | ||
| subtitleStyle={styles.subtitle} | ||
| /> | ||
| <View style={styles.commentsContainer}> | ||
| <Icon name="comment" type="octicon" size={18} color={colors.grey} /> | ||
| <Text style={styles.comments}> | ||
| {issue.comments} | ||
| </Text> | ||
| export const IssueListItem = ({ type, issue, navigation, language }: Props) => { | ||
| return ( | ||
| <TouchableHighlight | ||
| style={issue.state === 'CLOSED' && styles.closedIssue} | ||
| onPress={() => | ||
| navigation.navigate('Issue', { | ||
| issue, | ||
| isPR: type !== 'issue', | ||
| language, | ||
| })} | ||
| underlayColor={colors.greyLight} | ||
| > | ||
| <View style={styles.container}> | ||
| <ListItem | ||
| containerStyle={styles.listItemContainer} | ||
| title={issue.title} | ||
| subtitle={ | ||
| issue.state === 'OPEN' | ||
| ? translate('issue.main.openIssueSubTitle', language, { | ||
| number: issue.number, | ||
| user: issue.author.login, | ||
| time: moment(issue.createdAt).fromNow(), | ||
| }) | ||
| : translate('issue.main.closedIssueSubTitle', language, { | ||
| number: issue.number, | ||
| user: issue.author.login, | ||
| time: moment(issue.lastEditedAt).fromNow(), | ||
| }) | ||
| } | ||
| leftIcon={{ | ||
| name: getIconName(type, issue), | ||
| size: 36, | ||
| color: issue.state === 'OPEN' ? colors.green : colors.red, | ||
| type: 'octicon', | ||
| }} | ||
| hideChevron | ||
| titleStyle={styles.title} | ||
| subtitleStyle={styles.subtitle} | ||
| /> | ||
| <View style={styles.commentsContainer}> | ||
| <Icon name="comment" type="octicon" size={18} color={colors.grey} /> | ||
| <Text style={styles.comments}> | ||
| {issue.comments.totalCount} | ||
| </Text> | ||
| </View> | ||
| </View> | ||
| </View> | ||
| </TouchableHighlight>; | ||
| </TouchableHighlight> | ||
| ); | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉