Skip to content

[dev] Refactor user profile service#10

Open
ghost wants to merge 1 commit intodevfrom
features/refactor-user-profile-service
Open

[dev] Refactor user profile service#10
ghost wants to merge 1 commit intodevfrom
features/refactor-user-profile-service

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Feb 14, 2022

No description provided.

@ghost ghost requested review from bittu1028 and raoufr February 14, 2022 11:16
@ghost ghost self-assigned this Feb 14, 2022
@ghost ghost added the Do Not Merge label Feb 14, 2022
@ghost ghost requested a review from JLeeC February 16, 2022 05:39
@ghost
Copy link
Copy Markdown
Author

ghost commented Feb 16, 2022

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

@ghost
Copy link
Copy Markdown
Author

ghost commented Feb 28, 2022

@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.

@ghost ghost removed the Do Not Merge label Mar 15, 2022
return null;
}
// eslint-disable-next-line @typescript-eslint/no-unused-vars
import type { IUser } from './interfaces/user.interface';
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@ajaybushel we need to talk about our convention here make sure we are following the same on all repos including the mobile app. I will setup a meeting to sync up on that

@JLeeC
Copy link
Copy Markdown

JLeeC commented Oct 19, 2023

@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
Copy link
Copy Markdown
Collaborator

raoufr commented Oct 19, 2023

@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

@raoufr raoufr requested a review from mdjacobs October 19, 2023 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants