feat(login): add hook for login + interceptor #22

ZiyedB · 2021-02-14T20:51:29Z

Still need to do :

Refine the login component and the layout
Hook this to the api once live & having a way to detect the expiration
Potentially hooking for the refresh token
Address all the todos in the code
Adding unit tests for this

Changes

Added the login screen & route
Added some based for the authentication interceptor & redirection

Screenshots

ZiyedB · 2021-02-14T20:53:00Z

src/App.tsx

-import Ipfs from "./ipfs/Ipfs";
+function App(): JSX.Element {
+  // Handle user state
+  // Ask if we should have this instead directly in a utils instead


@NiiCoder should we extract this to a util ? I wasn't sure as it's using some state ( not sure about how this would be structured)

Better to have this in utils

ZiyedB · 2021-02-14T20:53:41Z

src/App.tsx

-        </Switch>
-      </BaseLayout>
-    </Router>
+    <UserContext.Provider value={{ isLoggedIn, login, logout }}>


Thought this would be the best to handle the user state. Open to suggestions, if there is a better way to do it with React

I think saving user information in localStorage is not a safe way. We have to ask the user to save user info("Remember me" checkbox).
And for the states that need to be used in several places, prefer to use redux.

ZiyedB · 2021-02-14T20:55:19Z

src/App.tsx

+        <BaseLayout>
+          <Switch>
+            <Route path="/login" component={Login} />
+            <ProtectedRoute path="/ipfs/:hash" component={Ipfs} />


I was wondering if we couldn't just handle this at a higher level, defining all the routes as protected seems overkill when we only have one route for not logged in ( /login )
But not sure if we can define two routers or instead conditional show one or the other ( if it's a good practice )

I think both are good. But to make code cleaner, prefer to have 2 routers.

ZiyedB · 2021-02-14T20:55:59Z

src/NavigationBar/NavigationBar.tsx

-        </div>
+        {userContext.isLoggedIn ? (
+          <div className="navigation-bar__user">
+            {/* TODO: Need to have this as a small menu showing up */}


This menu would show up all the users path, if later on we add profile, right now this is just for logout on click

ZiyedB · 2021-02-14T20:56:33Z

src/config/api.ts

@@ -0,0 +1,5 @@
+export const apis = {


Thought it would be good to handle everything in a config folder @NiiCoder
Apis urls, environment variables, etc.

ZiyedB · 2021-02-14T20:57:07Z

src/config/storage-names.ts

@@ -0,0 +1,3 @@
+export const storageNames = {
+  user: 'pl11',


this is just random "paralink11" just so it's not easy to guess when looking

ZiyedB · 2021-02-14T20:57:56Z

src/pages/Login/Login.scss

@@ -0,0 +1,72 @@
+$elevation-card: 0 0 1px 0 rgba(8, 11, 14, 0.6), 0 16px 16px -1px rgba(8, 11, 14, 0.1);


I am gonna create a ticket to define a theme & variables to use accross the app & a global style. Right now this is using a lot of "custom" things. But if we can come up with a global styling ( for inputs, buttons, etc. ) this would make it easier

ZiyedB · 2021-02-14T20:58:23Z

src/pages/Login/Login.tsx

+  from: string;
+}
+
+const Login = (props: RouteComponentProps<{}, {}, LoginProps>): JSX.Element => {


I need to add more comments but this is a base for now

ZiyedB · 2021-02-14T20:59:58Z

src/services/interceptor/auth.ts

+paralinkApi.interceptors.request.use(
+  (config: any) => {
+    // Check for user token
+    const accessToken = localStorage.getItem(storageNames.user);


Two things here where I need your input @NiiCoder

Should we use directly the user context instead ( and store the token in it )?

Should we store the token in the cookies or local storage is fine ? I went for local storage but cookies could work and we could make it 'more' secure in case

Oh, this is what I was saying.

store

cookie

ZiyedB · 2021-02-16T20:43:11Z

Added the hook form & validation, it would be useful for other features later on ( like that query builder )

@NiiCoder note I decide to actually keep things as they are for the moment for those reasons :

Redux is a bit overkill for now, we are not storing the user information for now and only having a token. The user context should be more than enough. If we were to have more data shared across the app it would be definitely worth adding that.
I've created this here : https://github.com/ZiyedB/paralink-ui/tree/TEST-WithRedux which is basically this PR with redux. ( a bit of struggle there ) but decided to keep things as they are now to make it less boilerplate. Also probably easier to test with this conf
I am also keeping it for now on the local storage for testing. Normally the set cookie would be directly set from server. This is to avoid issues with other browsers ( like Safari ). Once we have the api hooked up would check on that and add it. I like your idea about remember me ( probably something we could do for the refresh token )

Frenkiee · 2021-02-25T11:15:09Z

src/pages/Login/Login.tsx

+      .finally(() => {
+        // TODO: login loading should be set to false here
+      });
+  };


In login, you are using both async/await and promise patterns. Also, you are saying that the function returns Promise<any> which is not true. In the case of async/await It returns Promise<void> in case of Promise it returns void.

I suggest one of the following implementations:
Promise pattern

const login = (data: LoginFormData): void => { console.log('data', data); paralinkApi .get('https://jsonplaceholder.typicode.com/todos') .then(() => { const token = 'faketokenhere'; userContext.login(token); }) .catch((err: any) => console.error(err)) .finally(() => {}); };

Async/await pattern

const login = async (data: LoginFormData): Promise<void> => { console.log('data', data); try { var token = await paralinkApi.get('https://jsonplaceholder.typicode.com/todos') token = 'faketokenhere'; userContext.login(token); } catch (e) { console.error(e); } };

Here are some suggestions when to use one or another:
https://levelup.gitconnected.com/async-await-vs-promises-4fe98d11038f

ZiyedB commented Feb 14, 2021

View reviewed changes

ZiyedB self-assigned this Feb 14, 2021

ZiyedB added the help wanted Extra attention is needed label Feb 14, 2021

ZiyedB added 2 commits February 15, 2021 07:56

feat(login): add hook for login + interceptor

34f5931

add check on inputs

e116fc7

ZiyedB force-pushed the ISSUE-7 branch from 2ecb370 to e116fc7 Compare February 16, 2021 20:37

ZiyedB added enhancement New feature or request and removed help wanted Extra attention is needed labels Feb 18, 2021

Frenkiee reviewed Mar 2, 2021

View reviewed changes

feedback

c812b06

ZiyedB mentioned this pull request Mar 4, 2021

Issue 7 #28

Open

ZiyedB force-pushed the ISSUE-7 branch from 665b350 to c812b06 Compare March 4, 2021 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(login): add hook for login + interceptor #22

feat(login): add hook for login + interceptor #22

ZiyedB commented Feb 14, 2021

ZiyedB Feb 14, 2021

ghost Feb 14, 2021

ZiyedB Feb 14, 2021

ghost Feb 14, 2021

ZiyedB Feb 14, 2021

ghost Feb 14, 2021

ZiyedB Feb 14, 2021

ghost Feb 14, 2021

ZiyedB Feb 14, 2021

ZiyedB Feb 14, 2021

ZiyedB Feb 14, 2021

ZiyedB Feb 14, 2021

ZiyedB Feb 14, 2021

ghost Feb 14, 2021

ZiyedB commented Feb 16, 2021

Frenkiee Feb 25, 2021

		@@ -0,0 +1,72 @@
		$elevation-card: 0 0 1px 0 rgba(8, 11, 14, 0.6), 0 16px 16px -1px rgba(8, 11, 14, 0.1);

feat(login): add hook for login + interceptor #22

Are you sure you want to change the base?

feat(login): add hook for login + interceptor #22

Conversation

ZiyedB commented Feb 14, 2021

Changes

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZiyedB commented Feb 16, 2021

Choose a reason for hiding this comment