[SOK-16] #8

gloginov · 2024-09-27T05:38:57Z

+add backendApi
+create axios interceptors
+add userReducer and create function for use yandex api +add env
*update SignIn.tsx for use backendApi
*update SignUp.tsx for use backendApi
*format and lint

Какую задачу решаем

packages/client/src/store/reducers/user-reducer.ts

k0ndratov

Крутая работа.

Оставил пару моментов, которые меня волнуют.

packages/client/src/layouts/private-layout.tsx

k0ndratov · 2024-09-27T10:15:18Z

packages/client/src/layouts/private-layout.tsx

+  }, [user])
+
+  // if the user is full, show page
+  return user === null ? <h1>Загрузка...</h1> : <Outlet />


Выглядит так, что если запрос не пройдет, то будет бесконечно "Загрузка...". Возможно стоит при ошибке в backendApi перебрасывать на <Error>, чтобы хотя бы понятно было, что дальше нет смысла ждать.

Он кидает на авторизацию при неудачном запросе, сделаю компонент прелоадера для таких целей.

k0ndratov · 2024-09-27T10:19:10Z

packages/client/src/store/reducers/user-reducer.ts

+import { AppDispatch, InferAppActions } from '@/store'
+import backendApi from '@/api/backendApi'
+
+export type UserType = {


Может такую штуку использовать?

type Nullable<T> = T | null

Не стал использовать. Не понимаю в чем преимущество. Поясни

k0ndratov · 2024-09-27T10:22:36Z

packages/client/src/store/reducers/user-reducer.ts

+      if (data) {
+        dispatch(actions.setUser(null))
+
+        window.location.href = '/'


А нельзя с помощью роутера перебросить, чтобы не ломать spa?

k0ndratov · 2024-09-27T10:26:43Z

packages/client/src/store/reducers/user-reducer.ts

+      if (data) {
+        dispatch(actions.setUser(null))
+
+        window.location.href = '/'


И еще момент, я не разбираюсь в реакте. Нормально ли, что redux управляет редиректами? Я просто думал, что его задача исключительно управлять состоянием приложения. А тут получается что он начинает реализовывать это состояние.

Или я тут заблуждаюсь?

Наверно не нормально. Переделал, логика в компоненте

k0ndratov · 2024-09-27T10:32:30Z

packages/client/src/api/backendApi.ts

+    if (error.response?.status === 401) {
+      localStorage.removeItem('user')
+      // save page where we get 401 and redirect after login
+      window.location.href = '/sign-in?redirectUrl=' + window.location.pathname


Может быть можно роутером перебросить?

Переделал на роутер

k0ndratov · 2024-09-27T10:34:47Z

packages/client/src/api/backendApi.ts

+      window.location.href = '/sign-in?redirectUrl=' + window.location.pathname
+    }
+
+    if (axios.isCancel(error)) return Promise.reject(error)


А есть смысл в этом условии, если все равно возвращается тоже самое?

k0ndratov · 2024-09-27T10:36:36Z

packages/client/src/api/backendApi.ts

+  function (error: AxiosError) {
+    // if app get response code 401 (died token), redirect user to sign-in form
+    if (error.response?.status === 401) {
+      localStorage.removeItem('user')


Возможно не стоит давать backendApi возможность манипулировать localStorage`ем? Начинаем как будто смешивать логику запросов и управления состоянием.

Возможно стоит сделать какой-то errorHandler, который опознает ошибку и вызовет нужный контролер.

Манипуляцию localstorage уберу

k0ndratov · 2024-09-27T10:37:49Z

packages/client/src/api/backendApi.ts

+    if (error.response?.status === 401) {
+      localStorage.removeItem('user')
+      // save page where we get 401 and redirect after login
+      window.location.href = '/sign-in?redirectUrl=' + window.location.pathname


И возможно тоже смешивается логика. Теперь получается, что backendApi управляет роутами и локал сторажем. Как будто много на себя берет, не?

Он берет на себя одну функцию - отловить 401 (что говорит о том что авторизация слетела) и что-то сделать. Подумаю как оформить

k0ndratov · 2024-09-27T10:40:01Z

packages/client/src/components/ui/Button/Button.tsx

-  href?: string
+  className?: string | undefined
+  useFixWidth?: boolean | undefined
+  href?: string | undefined


Не будет чуть путацины с href`ом для кнопки? Вроде ожидается, что кнопка делает экшены, а ссылки перебрасывают.

Тут ты наверно прав. Уберу href оставлю только событие

Хороший поинт от @k0ndratov 👍

Timur233

Привет! Отличная работа! Сейчас попробую затянуть к себе и сделать кнопку "профиль" в шапке, с именем пользователя ))

У меня пара комментов:

Давай запретим посещение страниц, авторизации и регистрации авторизованным пользователям.
И я бы убрал ненужные комментарии в коде, пример ниже.

// if user is empty call backend
if (user === null) { dispatch(getUser()) }

Timur233 · 2024-09-28T10:56:22Z

packages/client/src/layouts/private-layout.tsx

+  useEffect(() => {
+    // if user is empty call backend
+    if (user === null) {
+      dispatch(getUser())


Обратил внимание, что при загрузке страницы запрос улетает 2 раза подряд.

Дважды улетает только в dev из-за strict mode
https://legacy.reactjs.org/docs/strict-mode.html

Timur233 · 2024-09-28T11:02:47Z

packages/client/src/layouts/private-layout.tsx

+  }, [user])
+
+  // if the user is full, show page
+  return user === null ? <h1>Загрузка...</h1> : <Outlet />


Он кидает на авторизацию при неудачном запросе, сделаю компонент прелоадера для таких целей.

Timur233 · 2024-09-28T11:09:49Z

packages/client/src/api/backendApi.ts

+    if (error.response?.status === 401) {
+      localStorage.removeItem('user')
+      // save page where we get 401 and redirect after login
+      window.location.href = '/sign-in?redirectUrl=' + window.location.pathname


Чуть смущает camelCase в гет параметрах. Это не правка, просто коммент.

Убрал вообще этот код. Сделал иначе

+add backendApi +create axios interceptors +add userReducer and create function for use yandex api +add env *update SignIn.tsx for use backendApi *update SignUp.tsx for use backendApi *format and lint

*fix lint error *add types from yandex api for user

*rename UserReducer to AuthReducer *rename methods in auth-reducer.ts -remove origin host from methods in auth-reducer.ts because base url set in backendApi

*update backendApi *change reducer for async/await *redirects with router (spa) +add toast for error message

*fix format *fix lint

MikeDevice

Ты молодец. Большая работа и это действительно сложная задачка. Описал всякие нюансы, как можно сделать лучше, но критических замечаний нет.

Вообще глобальная рекомендация:

Из реакт компонента кидать один экшн dispatch(login()), например, а в санке уже ловить этот экшн и отправлять запросы на сервер там. Так логика будет чище.
Ну и меньше копипасты и больше обобщений!

MikeDevice · 2024-10-02T09:07:45Z

package-lock.json

Интересно, что у вас в проекте есть и package-lock, и yarn.lock. Обычно используется что-то одно из двух. Возможно, у вас в команде кто-то устанавливает зависимости через npm install, а кто-то через yarn. Нужно разобраться с этим придти к единообразию.

MikeDevice · 2024-10-02T09:09:27Z

packages/client/src/api/backendApi.ts

+import axios from 'axios'
+
+const instance = axios.create({
+  baseURL: import.meta.env.VITE_AUTH_URL,


Круто, что такие штуки унесены в переменные окружения 👍

Отдельный лайк за абстракцию в виде backendApi, а не использование чистого axios во всех местах

MikeDevice · 2024-10-02T09:11:19Z

packages/client/src/app/App.test.tsx

-/* eslint-disable */
-import App from './App'
-import { render, screen } from '@testing-library/react'
+// import App from './App'


А что тут такое? Почему ты закоментил? Этот тест сейчас не запускается никогда?

MikeDevice · 2024-10-02T09:12:09Z

packages/client/src/components/ui/Button/Button.tsx

-  href?: string
+  className?: string | undefined
+  useFixWidth?: boolean | undefined
+  href?: string | undefined


Хороший поинт от @k0ndratov 👍

MikeDevice · 2024-10-02T09:13:06Z

packages/client/src/components/ui/Button/Button.tsx

+  useFixWidth?: boolean | undefined
+  href?: string | undefined
+  /* eslint-disable @typescript-eslint/no-explicit-any */
+  onClick?: (() => Promise<void>) | (() => any) | undefined


ESlint не зря ругается. Лучше вместо any использовать unknown, а ещё лучше чётко прописывать нужный тип.

Пытаюсь понять, а зачем три типа подаёшь? Смотри, так как у тебя после onClick идёт вопросительный знак, значит тип undefined можно смело удалить, останется два, но всё равно не пойму, зачем тут промис.

Ты просто кидаешь в button функцию onClick. Нативная кнопка промис не возвращает.

Нужно исправить это Вообще любой такой onClick обычно имеет тип (): void или такой же, только с параметром event внутри

MikeDevice · 2024-10-02T09:27:47Z

packages/client/src/pages/SignUp/SignUp.tsx

+        </label>
+        <Button
+          text={'зарегистрироваться'}
+          useFixWidth={true}


Suggested change

useFixWidth={true}

useFixWidth

Можно просто без true

MikeDevice · 2024-10-02T09:29:40Z

packages/client/src/store/index.ts

+import { combineReducers } from '@reduxjs/toolkit'
+
+const rootReducer = combineReducers({
+  AuthReducer,


Можно лучше: обычно пишут с маленькой буквы:

Suggested change

AuthReducer,

authReducer: AuthReducer,

MikeDevice · 2024-10-02T09:30:25Z

packages/client/src/store/index.ts

+export type RootState = ReturnType<typeof store.getState>
+// Inferred type: {posts: PostsState, comment: CommentsState, users: UsersState}
+export type AppDispatch = typeof store.dispatch
+export const useAppDispatch = useDispatch.withTypes<AppDispatch>()
+export type InferAppActions<T> = T extends {
+  [keys: string]: (...args: unknown[]) => infer U
+}
+  ? U
+  : never


Воу! прикольно! 👍

MikeDevice · 2024-10-02T09:32:20Z

packages/client/src/store/reducers/auth-reducer.ts

+}
+
+const initialState = {
+  user: null as unknown as UserType,


так обычно плохо делать. Лучше создать тип:

type InitialState = { user: UserType | null } const initialState: InitialState = { ... }

MikeDevice · 2024-10-02T09:34:16Z

packages/client/src/store/reducers/auth-reducer.ts

+)
+
+export type ActionsType = InferAppActions<typeof actions>
+export default AuthReducer


Можно лучше: Все санки похожи между собой. Рекомендую написать базовый санк, который ба содержал в себе обработку ошибок, и потом его использовать во всех местах.

*fix some comment for PR

*format *fix lint

gloginov force-pushed the SOK-16_authorization branch from fcac281 to 4110c13 Compare September 27, 2024 05:49

gloginov requested review from k0ndratov and Timur233 September 27, 2024 06:11

VladToby reviewed Sep 27, 2024

View reviewed changes

gloginov requested a review from VladToby September 27, 2024 08:40

VladToby approved these changes Sep 27, 2024

View reviewed changes

gloginov force-pushed the SOK-16_authorization branch from ab29ccb to 80fb9c2 Compare September 27, 2024 10:40

k0ndratov reviewed Sep 27, 2024

View reviewed changes

Timur233 requested changes Sep 28, 2024

View reviewed changes

Timur233 approved these changes Sep 30, 2024

View reviewed changes

gloginov added 4 commits October 1, 2024 21:41

[SOK-16]

057aab9

+add backendApi +create axios interceptors +add userReducer and create function for use yandex api +add env *update SignIn.tsx for use backendApi *update SignUp.tsx for use backendApi *format and lint

[SOK-16]

cf1f851

*fix lint error *add types from yandex api for user

[SOK-16]

d993cc1

*rename UserReducer to AuthReducer *rename methods in auth-reducer.ts -remove origin host from methods in auth-reducer.ts because base url set in backendApi

[SOK-16]

9960d7c

*update backendApi *change reducer for async/await *redirects with router (spa) +add toast for error message

gloginov force-pushed the SOK-16_authorization branch from bc559fc to 9960d7c Compare October 1, 2024 16:42

[SOK-16]

0234024

*fix format *fix lint

k0ndratov self-requested a review October 1, 2024 22:39

k0ndratov approved these changes Oct 1, 2024

View reviewed changes

MikeDevice approved these changes Oct 2, 2024

View reviewed changes

gloginov added 2 commits October 2, 2024 15:29

[SOK-16]

6dcb54e

*fix some comment for PR

[SOK-16]

94da985

*format *fix lint

gloginov merged commit 2def654 into develop Oct 2, 2024
1 check passed

[SOK-16] #8

[SOK-16] #8

Conversation

gloginov commented Sep 27, 2024

k0ndratov left a comment

Choose a reason for hiding this comment

k0ndratov Sep 27, 2024 • edited Loading

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

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

Timur233 left a 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

MikeDevice left a 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

k0ndratov Sep 27, 2024 •

edited

Loading