Nicolas de Lima Application - Fullstack

deLimaNicolas commented 3 years ago

Hey there!

I hope you`re good

Please check the Readme, I wrote some introduction to help you with this review!

I`m waiting for cool tips and insights on my code now, good luck and good code!

REPO -- https://github.com/deLimaNicolas/test-fullstack

rankun203 commented 3 years ago

@deLimaNicolas Thanks for the PR! A few topics I wanna discuss with you:

For the Routes + Compose Generator (error handling & service encapsulation) design, is there any room for simplify / improve it? As I understand you haven't completely abstracted the network related dependencies out of the services layer as "Compose Generator" is trying to do.
I saw your notes on the refresh token, if one day you decided to do it properly, how'd you like to do it from the FE side?

deLimaNicolas commented 3 years ago

@deLimaNicolas Thanks for the PR! A few topics I wanna discuss with you:

1. For the Routes + Compose Generator (error handling & service encapsulation) design, is there any room for simplify / improve it? As I understand you haven't completely abstracted the network related dependencies out of the services layer as "Compose Generator" is trying to do.

2. I saw your notes on the refresh token, if one day you decided to do it properly, how'd you like to do it from the FE side?

@rankun203 Hey there! Good points, let's go then

About the Compose Generator:

I think there is a lot of space for improvements since this would be a central point of the application, it is important to be scalable.
I was thinking about creating a method responsible for extracting the Request's info, so we wouldn't need to pass this object to the next layer, this way the rest of the code doesn't need to care about anything but the business logic itself.
By doing this way, I would be able to enforce the structure of each request, validate it, and also apply optional middlewares. And all this, before the function requested even start.
Actually, I did something similar to this, but it was in this Lamda Function Repo, so a different scenario, I'm linking some images of the implementation to represent it.
And I also had the idea to use this ComposeGenerator as a Middleware, and break its logic among different middlewares in the Application Manager file, but since I was not so sure about it, I decided to go as a "Util"
For simplifying, maybe I could create some abstraction to make the use easier, something like the "init" method in the images I'm sending.

Sorry if this isn't what you were expecting, I would love to get your opinion on how to improve this, it is a strategy that I like to implement as you can see, so any feedback would be really valuable for me :)

OBS: I changed some names so I wouldn't expose anything

Imp Imp2

Now about the refresh token.

I'm thinking about this problem with a simple approach, nothing too sophisticated.

All the tokens would be informed using the headers
I would have the token itself being generated with some expiration time
I would also have the refresh_token, being generated with a greater expiration time
When the Frontend requests the token it also receives the refresh_token
The Frontend has info about the token expiration time, this way it can automatically make a request for a new one when it expires
In order to get a new token, the Frontend must send the refresh_token together in the request
A new refresh_token is generated each time this happens, and the Frontend has to save it
If the refresh_token expires, then the Frontend must force the user to log in again
I guess that would be the main idea, and of course, some cool features would appear in a refinement meeting.
- Maybe using different secrets for the refresh_token and the token.
- Maybe change the secrets from time to time
- Perhaps use some encryption method of the browser to store this access data
- Well, I can go without a stop here, so I think the main Idea is enough for this answer

I hope this could solve your questions! But if that is not the case, feel free to ask more

Thanks!

rankun203 commented 3 years ago

@deLimaNicolas Hi, thank you for your detailed response! I see you are pretty passionate and creative on researching different ways to improve your code architecture.

IMO, Express's built-in middleware mechanism by itself is pretty much enough to add extra logic before and after a request reaches to our controller in the cases you mentioned, so I haven't see enough benefit this particular approach brought to the table but added complexity. We won't actually go deep on this topic in this discussion, but if it was me I'd do the same thing probably. Just to explore the possible ways of building stuff.
I was asking more about how to implement a token refresh mechanism, supporting both refresh on demand (by network request) and automatic background refresh, this kind of things. But your explanation on how token & refresh token works is also good.

I think that's enough info from my side, have a good day :-)

deLimaNicolas commented 3 years ago

@rankun203 Totally agree! I could just move this logic to the Middlewares folder, as I said before it is something that was in my head, but I over-engineered it this time :/

Thanks for your feedback! Have a Good Rest

Wiredcraft / test-fullstack