Skip to content

Conversation

@well-balanced
Copy link
Contributor

@well-balanced well-balanced commented Apr 10, 2021

Description

Related issue #277 and also PR #283

implementing below

Migrate all old endpoints to /v2 and update frontend accordingly

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

@github-actions github-actions bot added the api API related changes ( api folder ) label Apr 10, 2021
@well-balanced
Copy link
Contributor Author

I just created this PR for feedback. if I get feedback from the member, this PR gonna be closed.

I have some questions

  • are all endpoints gonna return DTO objects like a GetContributorsResponseDto ?
  • I handled errors in the GithubService's method using try catch.
    if you don't like it can you suggest to me another best practice?
  • in this migration case, the old api just returns GitHub API's response. Do I have to make Dto object like a GithubUserDto?

please give me a feedback thx 🙏

@well-balanced well-balanced changed the title Add GitHub Controller Add new GitHub Controller Apr 10, 2021
@ZibanPirate
Copy link
Member

thank you @well-balanced for your PR !

Answering your questions:

  • are all endpoints gonna return DTO objects like a GetContributorsResponseDto ?

Yes, basically we need two things for typing an endpoint:

  • first, the return type; an interface to strongly type our controller.
  • second, the return DTO class, which is needed for OpenApi and swagger to generate automatic documentation for /v2/docs route.
    so, instead of doing each one separately, we just create a DTO class that also act as a return type, so two birds in one stone, while we apply the DRY principle.
  • I handled errors in the GithubService's method using try catch.
    if you don't like it can you suggest to me another best practice?

Error should be thrown, don't use try catch, as we have an error handler in error.ts

  • in this migration case, the old api just returns GitHub API's response. Do I have to make Dto object like a GithubUserDto?

please give me a feedback thx 🙏

So, unlike the controller endpoint, calling third party APIs like Github, doesn't need to be documented,however the axios response definitely need to be strongly typed, so.., we just need a regular typescript interface/type here ;)

I just created this PR for feedback. if I get feedback from the member, this PR gonna be closed.

As for this PR, we can merge it, we just need some small changes, no need to waste your effort by closing it ;)
i will give you some feedback on the PR itslef, once fixed we can merge!

@well-balanced
Copy link
Contributor Author

well-balanced commented Apr 10, 2021

@ZibanPirate
I fixed some parts

  • Delete try catch expression
  • Add GitHub repo API Response interface into types.ts

Could you feedback to me again specifically?

I think file location will be better to be src/github-repository. what about u?

@ZibanPirate
Copy link
Member

well, since you already worked on #286 and merged both #290 and #289 , i believe there is no point in leaving this open, so closing ...
if you want to re-open it again, feel free to do so @well-balanced , and again thank you so much for your contribution!

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

Labels

api API related changes ( api folder )

Projects

Status: Archived

Development

Successfully merging this pull request may close these issues.

2 participants