search

saddlebackdev / hc-mobile-app-core-services

MIT License
1 stars 0 forks source link

[dev] Refactor user profile service #10

Open ajaybushel opened 2 years ago

ajaybushel commented 2 years ago

Just noticed an open issue #9 and pretty sure that this PR should partially solve it.

ajaybushel commented 2 years ago

@raoufr I tried to integrate the api utils in the user profile service, but it seems to be complicating things. The way ApiUtils is designed is to have its instances created for each service.

const userProfileApi = createApi(baseUrl);

For the consumers, to be able to pass the baseUrl to the createApi method, the service will have to have a "init(baseUrl)" method which will assign the baseUrl value to the createApi method. As far as I can see, doing so will only create more complexities, when for example, needing to pass an access token in the headers, or maybe applying middlewares to the userProfileApi instance.

So for now, we can leave the service as is. Whats remaining in this PR is to add the return types on multiple methods. I think @bittu1028 is going to take care of that.

JLeeC commented 11 months ago

@raoufr I tried to integrate the api utils in the user profile service, but it seems to be complicating things. The way ApiUtils is designed is to have its instances created for each service.

const userProfileApi = createApi(baseUrl);

For the consumers, to be able to pass the baseUrl to the createApi method, the service will have to have a "init(baseUrl)" method which will assign the baseUrl value to the createApi method. As far as I can see, doing so will only create more complexities, when for example, needing to pass an access token in the headers, or maybe applying middlewares to the userProfileApi instance.

So for now, we can leave the service as is. Whats remaining in this PR is to add the return types on multiple methods. I think @bittu1028 is going to take care of that.

I don't love having a different patternfor this. I am ok with it now to move us forward, as this is relatively abstracted, but teh whole point of apiUtils was to have one common place to make any types of api calls, INCLUDING handling exception, cleaningup, etc. I understand your challenge of adding things like tokens into the headers ... but we shoudl be updating apiUtils.ts to support this, as opposed to doing this one off

If this is pressing, i am ok moving this in first as, like i said, it is abstracted enough. @ajaybushel and @bittu1028 , if we have cycles, i'd like for us to ensure we get api utils to support our common microservice calls, instead of having all these different patterns.

raoufr commented 11 months ago

@raoufr I tried to integrate the api utils in the user profile service, but it seems to be complicating things. The way ApiUtils is designed is to have its instances created for each service.

const userProfileApi = createApi(baseUrl);

For the consumers, to be able to pass the baseUrl to the createApi method, the service will have to have a "init(baseUrl)" method which will assign the baseUrl value to the createApi method. As far as I can see, doing so will only create more complexities, when for example, needing to pass an access token in the headers, or maybe applying middlewares to the userProfileApi instance. So for now, we can leave the service as is. Whats remaining in this PR is to add the return types on multiple methods. I think @bittu1028 is going to take care of that.

I don't love having a different patternfor this. I am ok with it now to move us forward, as this is relatively abstracted, but teh whole point of apiUtils was to have one common place to make any types of api calls, INCLUDING handling exception, cleaningup, etc. I understand your challenge of adding things like tokens into the headers ... but we shoudl be updating apiUtils.ts to support this, as opposed to doing this one off

If this is pressing, i am ok moving this in first as, like i said, it is abstracted enough. @ajaybushel and @bittu1028 , if we have cycles, i'd like for us to ensure we get api utils to support our common microservice calls, instead of having all these different patterns.

I second @JLeeC's concern. I think we should hold off on this PR and discuss the possibility of enhancing the ApiUtils service, if really needed, then come back here and assess the proposed changes in this PR at that point. If there is a real urgency to get this PR merged in this state, please let us know and let's discuss.

cc @mdjacobs @bittu1028 @JLeeC