Skip to content
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

Merged
merged 1 commit into from
Dec 28, 2018
Merged

add client-side router #433

merged 1 commit into from
Dec 28, 2018

Conversation

smacker
Copy link
Contributor

@smacker smacker commented Dec 26, 2018

Ref: #399

@carlosms carlosms mentioned this pull request Dec 26, 2018
@carlosms
Copy link
Contributor

link to issue: #399

@carlosms
Copy link
Contributor

Continuing the conversation from #425.
The code looks good and not too complex. And it opens the door for a future UI design with proper routes like /org-name/settings, /org-name/repo-name/settings.

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 /login and /logout routes in the address bar, are there any downsides to not use the router, @smacker? How would the auth management work?

@smacker
Copy link
Contributor Author

smacker commented Dec 27, 2018

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 Token.delete() && window.reload().

I see this advantages of adding the router on the current stage (from the biggest to the smallest in my option):

  • It might be much of a headache to add a router to an existing project that didn't have it before. Due to synchronization of the URL and internal state.
  • A router naturally introduces abstraction of pages and separation of them from components. I find it useful while building web applications.
  • Nice urls

And the disadvantages:

  • it requires a bit more code due to better structure / new page & route abstractions

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]>
Copy link
Contributor

@dpordomingo dpordomingo left a 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() {
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

@carlosms carlosms left a comment

Choose a reason for hiding this comment

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

👍

@smacker smacker merged commit d91461d into src-d:master Dec 28, 2018
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.

None yet

3 participants