-
Notifications
You must be signed in to change notification settings - Fork 772
refactor(redux): Switch to redux middleware & normalizr for Rest #457
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 32 commits
1b057fc
f0948d0
0f5746d
b710e80
b52977a
ae00beb
de4ab42
83e92b5
a24f64e
0ab34f2
29fd81f
a7cd9e1
82495a6
455229d
cc41686
166d6a9
57e2f74
440e182
9f58286
5e29560
77e40a2
9c07e82
a54da4b
2304b4b
e7a87f1
4703a88
da4e6b1
8055862
df3515f
1d65a0a
8c24f81
cafa620
9cbabee
5e576db
c12429b
5c1d82e
10a24ca
e2e9ca3
ac036ec
34d6d28
82be98a
40429ca
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 |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| import { createActionSet } from 'utils'; | ||
|
|
||
| export const ACTIVITY_GET_EVENTS_RECEIVED = createActionSet( | ||
|
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 do you think about using
Member
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 way it's done in this PR, actions are barely seeable already :
What would be the benefit from using
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. Here's a PR that uses
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. Redux Actions will reduce verbosity introduced by default by Redux. You can see the motivation here https://redux-actions.js.org/docs/introduction/Motivation.html
Member
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 understand the motivation here, but against master, not this PR :/ |
||
| 'ACTIVITY_GET_EVENTS_RECEIVED' | ||
| ); | ||
| export const ACTIVITY_GET_NOTIFICATIONS = createActionSet( | ||
| 'ACTIVITY_GET_NOTIFICATIONS' | ||
| ); | ||
|
|
||
| export const COUNT_ACTIVITY_GET_NOTIFICATIONS = createActionSet( | ||
| 'COUNT_ACTIVITY_GET_NOTIFICATIONS' | ||
| ); | ||
| export const ACTIVITY_MARK_NOTIFICATION_THREAD_AS_READ = createActionSet( | ||
| 'ACTIVITY_MARK_NOTIFICATION_THREAD_AS_READ' | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| export * from './orgs'; | ||
| export * from './search'; | ||
| export * from './activity'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import { createActionSet } from 'utils'; | ||
|
|
||
| export const ORGS_GET_BY_ID = createActionSet('ORGS_GET_BY_ID'); | ||
| export const ORGS_GET_REPOS = createActionSet('ORGS_GET_REPOS'); | ||
|
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 it used? And for all these actions: can use a naming convention that was earlier?
Member
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. When I started this PR, I was trying to have the following namespace set up: The goal is to namespace the API by entities. It seems to me more clear and well dispatched. But I was thinking about renaming |
||
| export const ORGS_GET_MEMBERS = createActionSet('ORGS_GET_MEMBERS'); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import { createActionSet } from 'utils'; | ||
|
|
||
| export const SEARCH_GET_REPOS = createActionSet('SEARCH_GET_REPOS'); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| export class Client { | ||
| Method = { | ||
| GET: 'GET', | ||
| HEAD: 'HEAD', | ||
| PUT: 'PUT', | ||
| DELETE: 'DELETE', | ||
| PATCH: 'PATCH', | ||
| POST: 'POST', | ||
| }; | ||
|
|
||
| fetch = async ( | ||
| url, | ||
| { | ||
| method = this.Method.GET, | ||
| schema = null, | ||
| normalizrKey = null, | ||
| headers = {}, | ||
| }, | ||
| params = {} | ||
| ) => { | ||
| let finalUrl; | ||
|
|
||
| if (params.url) { | ||
| // a different url was provided, use it instead (paginated) | ||
| finalUrl = params.url; | ||
| } else { | ||
| finalUrl = url; | ||
| // add explicitely specified parameters | ||
| if (params.per_page) { | ||
| finalUrl = `${finalUrl}${finalUrl.indexOf('?') !== -1 | ||
|
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. I think we can use here the method includes = |
||
| ? '&' | ||
| : '?'}per_page=${params.per_page}`; | ||
| } | ||
| } | ||
|
|
||
| if (finalUrl.indexOf(this.API_ROOT) === -1) { | ||
|
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. The same, in order do improve the readability, we can use includes |
||
| finalUrl = `${this.API_ROOT}${finalUrl}`; | ||
| } | ||
|
|
||
| const parameters = { | ||
| method, | ||
| headers: { | ||
| 'Cache-Control': 'no-cache', | ||
| ...this.authHeaders, | ||
|
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. Hey, where 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. 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 |
||
| ...headers, | ||
| }, | ||
| }; | ||
|
|
||
| return fetch(finalUrl, parameters) | ||
| .then(response => { | ||
| // analyze headers for pagination, rates, etc | ||
| return { | ||
| response, | ||
| schema, | ||
| normalizrKey, | ||
| }; | ||
| }) | ||
| .catch(error => error); | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export * from './client'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| import { Client } from '../base'; | ||
| import Schemas from './schemas'; | ||
|
|
||
| export class GitHub extends Client { | ||
| API_ROOT = 'https://api.github.com/'; | ||
|
|
||
| setAuthHeaders = token => { | ||
| this.authHeaders = { Authorization: `token ${token}` }; | ||
| }; | ||
|
|
||
| getNextPageUrl = 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. Proposal: I think that if pass the
Member
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 correct for Github, but for Gitlab for example, next link information is contained in the JSON response. This is why I decided to pass |
||
| const link = response.headers.get('link'); | ||
|
|
||
| if (!link) { | ||
| return null; | ||
| } | ||
|
|
||
| const nextLink = link.split(',').find(s => s.indexOf('rel="next"') > -1); | ||
|
|
||
| if (!nextLink) { | ||
| return null; | ||
| } | ||
|
|
||
| return nextLink.split(';')[0].slice(1, -1); | ||
| }; | ||
|
|
||
| /** | ||
| * The organizations endpoint | ||
| */ | ||
| orgs = { | ||
| /** | ||
| * Gets an organization by its id | ||
| * | ||
| * @param {string} orgId | ||
| */ | ||
|
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. There is no description for
Member
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. Description for 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? |
||
| getById: async (orgId, params) => { | ||
| return this.fetch( | ||
| `orgs/${orgId}`, | ||
| { | ||
| schema: Schemas.ORG, | ||
| }, | ||
| params | ||
| ).then(struct => struct); | ||
| }, | ||
| /** | ||
| * Gets organization members | ||
| * | ||
| * @param {string} orgId | ||
| */ | ||
| getMembers: async (orgId, params) => { | ||
| return this.fetch( | ||
| `orgs/${orgId}/members`, | ||
| { | ||
| schema: Schemas.USER_ARRAY, | ||
| }, | ||
| params | ||
| ).then(struct => { | ||
|
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. This a code style minor thing but you can remove the return key using: |
||
| return { | ||
| ...struct, | ||
| nextPageUrl: this.getNextPageUrl(struct.response), | ||
| }; | ||
| }); | ||
| }, | ||
| /** | ||
| * Gets organization members | ||
| * | ||
| * @param {string} orgId | ||
| */ | ||
| getRepos: async (orgId, params) => { | ||
| return this.fetch( | ||
| `orgs/${orgId}/repos`, | ||
| { | ||
| schema: Schemas.REPO_ARRAY, | ||
| }, | ||
| params | ||
| ).then(struct => { | ||
| return { | ||
| ...struct, | ||
| nextPageUrl: this.getNextPageUrl(struct.response), | ||
| }; | ||
| }); | ||
| }, | ||
| }; | ||
|
|
||
| /** | ||
| * The activity endpoint | ||
| */ | ||
| activity = { | ||
| /** | ||
| * Gets received events | ||
| * | ||
| * @param {string} userId | ||
| */ | ||
| getEventsReceived: async (userId, params) => { | ||
|
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. Maybe rename to simply
Member
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. 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 |
||
| return this.fetch( | ||
| `users/${userId}/received_events`, | ||
| { | ||
| schema: Schemas.EVENT_ARRAY, | ||
| }, | ||
| params | ||
| ).then(struct => { | ||
|
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. Don't really understand what's going on here fully 😞 You're passing
Member
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 explained this a bit more in my reply to your first review comment. Today, I'll be expanding this PR to the notifications screens, in order to proof test pure vs reduced api calls:
I'll let you know how it plays out! |
||
| return { | ||
| ...struct, | ||
| nextPageUrl: this.getNextPageUrl(struct.response), | ||
| }; | ||
| }); | ||
| }, | ||
| /** | ||
| * Get all notifications for the current user | ||
| * | ||
| * @param {boolean} all If true, show notifications marked as read. | ||
| * @param {boolean} participating If true, in which the user is directly participating or mentioned. | ||
| */ | ||
| getNotifications: async (all, participating, params) => { | ||
| const finalParams = { | ||
| per_page: 100, | ||
| ...params, | ||
| }; | ||
|
|
||
| return this.fetch( | ||
| `notifications?all=${all}&participating=${participating}`, | ||
| { | ||
| schema: Schemas.NOTIFICATION_ARRAY, | ||
| }, | ||
| finalParams | ||
| ).then(struct => { | ||
| return { | ||
| ...struct, | ||
| nextPageUrl: this.getNextPageUrl(struct.response), | ||
| }; | ||
| }); | ||
| }, | ||
| markNotificationThreadAsRead: async (id, params) => { | ||
| return this.fetch( | ||
| `notifications/threads/${id}`, | ||
| { | ||
| method: this.Method.PATCH, | ||
| }, | ||
| params | ||
| ).then(struct => { | ||
| return { | ||
| ...struct, | ||
| }; | ||
| }); | ||
| }, | ||
| }; | ||
|
|
||
| search = { | ||
| /** | ||
| * Search repositories | ||
| * | ||
| * @param {string} query | ||
| */ | ||
| getRepos: async (query, params) => { | ||
| return this.fetch( | ||
| `search/repositories?${query}`, | ||
| { | ||
| schema: Schemas.REPO_ARRAY, | ||
| normalizrKey: 'items', | ||
| }, | ||
| params | ||
| ).then(struct => { | ||
| return { | ||
| ...struct, | ||
| nextPageUrl: this.getNextPageUrl(struct.response), | ||
| }; | ||
| }); | ||
| }, | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export * from './client'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import { schema } from 'normalizr'; | ||
| import { initSchema, toTimestamp } from 'utils'; | ||
|
|
||
| import { userSchema } from './users'; | ||
| import { orgSchema } from './orgs'; | ||
| import { repoSchema } from './repos'; | ||
|
|
||
| export const eventSchema = new schema.Entity( | ||
| 'events', | ||
| { | ||
| actor: userSchema, | ||
| org: orgSchema, | ||
| repo: repoSchema, | ||
| }, | ||
| { | ||
| idAttribute: event => event.id, | ||
| processStrategy: entity => { | ||
|
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. I think that can be refactor in order to avoid
Member
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. Thanks a lot for your review man! Just went through your remarks and applied the suggestions. I didn't know about I just kept EventSchema like that for the moment, cause it still needs some work that may require that |
||
| const processed = initSchema(); | ||
|
|
||
| processed.id = entity.id; | ||
| processed.type = entity.type; // TODO: needs to be normalized in an Enum | ||
| processed.payload = entity.payload; // TODO: needs to be inspected for more nested entities (forkee) | ||
| processed.createdAt = toTimestamp(entity.created_at); | ||
|
|
||
| processed.actor = entity.actor; | ||
| processed.org = entity.org; | ||
| processed.repo = entity.repo; | ||
|
|
||
| return processed; | ||
| }, | ||
| } | ||
| ); | ||
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.
Perhaps it's better to use the full name here (
OrganizationRepositoryList), because the other screens start with theOrganization, not theOrg(to support consistency)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.
Absolutely