Skip to content

refactor(api): Api clean up#361

Merged
housseindjirdeh merged 12 commits intogitpoint:masterfrom
machour:api-banana-split
Sep 26, 2017
Merged

refactor(api): Api clean up#361
housseindjirdeh merged 12 commits intogitpoint:masterfrom
machour:api-banana-split

Conversation

@machour
Copy link
Copy Markdown
Member

@machour machour commented Sep 26, 2017

While looking at redux, I came accros a few facts with api/index.js :

  • fetch() is being called in a lot of functions, inside and outside api/index.js. Some actions files also borrowed the accessTokenParameters* helpers.
  • weird function naming: fetchUrl for GET returning json, and fetchNormal for GET returning a Response
  • duplicate functions: fetchUrlCommentsHTML was in fact performing the exact same thing as fetchUrl
  • code was piling up and the file was getting unmaintainable.

This PR cleans up all this, and makes the API crystal clear, by introducing the v3 namespace:

v3 is a simple object representing our GitHub Rest API v3 client :

  • v3.call() is the only function that calls GitHub. It receives the url and parameters arguments for the fetch() call and returns a Response
  • v3.root is the root endpoint
  • v3.parameters() is the sole responsible for preparing the fetch() call parameters. It takes the accessToken (mandatory), the HTTP method (optional), the Accept header (optional) and a body for the request (optional), does its magic, and returns the correct object.
  • The rest of the object consist of helpers method using a clear nomenclature :
    httpVerb AcceptedResult ()

For example:

  • v3.get() performs a GET query and returns the Response
  • v3.getJson() performs a GET query and returns the .json() response
  • v3.head() performs a HEAD query and returns the Response
  • v3.postRaw() performs a POST request with the given object and returns the .text() response

I implemented all the needed helpers, and if a new combination arises (postHtml() for example), it should be added in v3 following this nomenclature.

I also adopted two enums, METHOD and ACCEPT to attempt to standardize things a bit more.

This clean up made obvious that fetchStarCount() was missing the accessToken, and that some weird calls were made by the watch/unwatch actions. I fixed the calls.

I did my best to test all the apps, but I might have missed a few spots, please let me know if it's the case!

Next step (once this is merged), is to re-dispatch all remaining functions (aside v3 of course) from api/index.js to the actions files where they belong.

Copy link
Copy Markdown
Member

@andrewda andrewda left a comment

Choose a reason for hiding this comment

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

This looks fantastic!

@lex111
Copy link
Copy Markdown
Member

lex111 commented Sep 26, 2017

@machour this is an amazing job! Could you please add method for executing a GraphQL query from PR #347?

And do you think we can use the plural for METHOD and ACCEPT constants?

@machour
Copy link
Copy Markdown
Member Author

machour commented Sep 26, 2017

@lex111 GraphQL should be in a v4 namespace, it will be added in another PR.
I'm trying to keep this PR as small and focused as possible for now to avoid side effects.

For the plurals, are you talking about changing METHOD to METHODS and ACCEPT to ACCEPTS, or do you mean something else ? 🤔

@lex111
Copy link
Copy Markdown
Member

lex111 commented Sep 26, 2017

Yes, you are right, GraphQL is already v4.

For the plurals, are you talking about changing METHOD to METHODS and ACCEPT to ACCEPTS, or do you mean something else

Correct, do you think?

@machour
Copy link
Copy Markdown
Member Author

machour commented Sep 26, 2017

@lex111 it seems more natural to me to use the singular form. The idea is that you are selecting one method of the possible choices in the enum. I always see that in java projects for example.

Copy link
Copy Markdown
Member

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

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

Wow this is so great, thank you so much @machour. This is going to smoothen things out so nicely once we start updating and using the v4 namespace for GraphQL.

Ran some regressions and it all looks smooth as silk - next up like you mentioned would be including each
of the fetch methods within the appropriate directories and ideally, we should be able to swap out
v3 to v4 thanks to this wrapper 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants