-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add client-side router #433
Conversation
link to issue: #399 |
Continuing the conversation from #425. But still, it adds bloat that I don't know if we want to add at this stage (basic UI was the key requirement). Besides losing the nice |
There are no technical limitations to do everything without a router. It works just fine on the previous PR. Most probably it would require only new logout method of App component and pass it down as props to every component that need to do logout (including failed api calls). And of course there are some more hacky solutions like just I see this advantages of adding the router on the current stage (from the biggest to the smallest in my option):
And the disadvantages:
As a conclusion maybe I wouldn't implement it right now if there were no requirements for clean url. But now when it's already here I see more benefits than harm by merging it. |
Signed-off-by: Maxim Sukharev <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I see it technically correct, and also reasonable to add a router as soon as possible.
I agree with Maxim that introducing it later, will cause more pain, and I see no pros on keep adding functionality without such a simple routing.
class AuthService { | ||
private _user: User | null = null; | ||
|
||
get token() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the getters in this class have return type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the type is defined on localStorage.getItem
method. Typescript is smart enough to figure it and define the type for getters automatically. We can add typing here just to avoid wrong type if we change the implementation of the getters. But at the same time if we change implementation that causes different automated type it will fail anyway at the place where we use this getter so it's not that beneficial.
I don't mind to define the type here manually. Let's just decided if we want to do it everywhere or not to be consistent. If we decide to add I would prefer to add it to any methods not only getters. With the only exception of life-cycle methods of react-components. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure right now. I guess if it's good enough for the strict
setting in tsconfig
we can keep using the inferred return; we can always add them later if we see we prefer the explicit types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Ref: #399