task-99: feat: внедрить OAuth #108

andreyvolokitin · 2021-09-20T18:03:52Z

No description provided.

DariaZherebtsova · 2021-09-21T08:48:41Z

src/store/reducers/userReducers.ts

-        state = { ...state, ...action.payload };
+        Object.assign(state, action.payload);


Антон мне сказал, что с state = { ...state, ...action.payload } - более красиво и читаемо

Я не уверен что это будет работать, там надо мутировать сам state, а тут будет создаваться новый объект

Я понял почему это не работало, надо возвращать новый объект, а не присваивать в state

В редаксе никогда не надо мутировать стейт, только возвращать новый объект)

В голом редаксе да. В тулките по дефолту immer включен, и уже был мутирующий код в редьюсерах на его основе. Ещё 1 причина разбираться с тем, как работает голый редакс

Хотя в тулките так можно работать, там же immer

Что касается Object.assign, то лучше спреды юзать. Стильно, модно, молодёжно и поведение супер ожидаемое

DariaZherebtsova · 2021-09-21T09:04:00Z

src/shared/utils/hooks/useProgress.ts

@@ -0,0 +1,23 @@
+import { useCallback, useState } from 'react';
+
+export default function useProgress(


Поясни, пожалуйста, что этот хук делает

Создаёт функцию запроса через useCallback и флаг ожидания через useState, возвращает их. Флаг ожидания используется для отображения состояния запроса, например для показа спиннера в кнопке, функция используется для запуска запроса, например в onClick кнопки

👍🏻
Выносить такие штуки в хук — хорошая идея, красавчик)

kotosha-real

Есть пара вопросов по userReducers, а так очень хороший пр)

kotosha-real · 2021-09-21T18:01:35Z

src/shared/utils/hooks/useOAuth.ts

+        } catch (e) {
+          throw new Error(e);
+        } finally {
+          window.history.replaceState(null, document.title, window.location.pathname);


❓
А зачем?

Там в url код остаётся, его надо убрать. Я думал как это лучше сделать, и когда (может лучше сразу же до начала авторизации его убрать, но тогда если авторизация будет неуспешна, а этот параметр был на самом деле вообще для другого, то получается уберём чужой параметр)

Это не супер критично на самом деле. Но если уже убираем — давай убирать

kotosha-real · 2021-09-21T18:16:32Z

src/pages/LoginPage/LoginPage.tsx

@@ -33,6 +36,19 @@ function LoginPage({ title }: GenericPageProps): JSX.Element {
      console.log('error', translateErrorMessage(err.message));
    }
  }, []);
+  const [isOAuthInProgress, startYandexOAuth] = useProgress(async () => {


🧐
Кажется, что window.location.replace убивает весь шарм неплохого хука — запрос за id будет очень коротким, по сути ты даже можешь не увидеть прогресс

Не таким уж и коротким (on my machine), для доп. шарма я даже ввёл вроде бы бесполезный второй параметр — keepProgress, т.е. он ещё будет показываться пока страница раздумывает перед редиректом, а запрос уже завершён. Ну и редирект в любом случае нужен, как и хук (только я засомневался насчёт него, его вроде бы можно использовать для любого запроса, но если внутри асинхронной функции будут зависимости, которые следовало бы указать в useCallback, то это уже нельзя будет сделать, хотя формально вроде бы уже и не нужно, т.к. единственная зависимость useCallback — сама функция)

Если useCallback имеет зависимости, то их нужно туда передать, иначе корректность вычислений внутри функции пострадает. Поинт хороший, юзабельность хука можно дополнить массивом зависимостей (то есть третьим аргументом)

kotosha-real · 2021-09-21T18:18:13Z

src/shared/utils/hooks/useProgress.ts

@@ -0,0 +1,23 @@
+import { useCallback, useState } from 'react';
+
+export default function useProgress(


👍🏻
Выносить такие штуки в хук — хорошая идея, красавчик)

kotosha-real · 2021-09-21T18:18:51Z

src/shared/utils/hooks/useProgress.ts

+ */
+export default function useProgress(
+  action: () => Promise<unknown>,
+  keepProgress = false


❓
А в чём юзабельность? Если передать true, то получится вечный спиннер

Тут описал: #108 (comment)

kotosha-real · 2021-09-21T18:20:25Z

src/store/reducers/userReducers.ts

+  const response = await api.getUser();
+
+  return response;


❗️
Такое безболезненно можно сокращать

Suggested change

const response = await api.getUser();

return response;

return await api.getUser();

Это сомнительный момент, который я пока не прояснил. return await считается антипаттерном, но не всегда. Надо раз и навсегда с этим разобраться

Вот тут есть о том, когда надо, а когда не стоит)

В конкретно этом случае эти участки кода идентичны, в правиле говорится о том же

Идентичны, и следуя правилу как бы "неправильны", т.е. надо писать return api.getUser(). Во многих случаях (у нас во всём коде подобные строчки/случаи) это будет вроде как нормально — если мы не обрабатываем ошибки, то возвращаем такой же промис, как и через await, просто он разрешится самостоятельно, а мы его в функции ждать не будем. Исключение когда мы деструктурируем результат const { val } = await api.method(), тут уж точно нужен await.

Но здесь у нас редьюсер, т.е. может ожидается какой-то особый возврат, хотя с await мы тот же промис возвращаем, просто который уже точно зарезолвится. Мне кажется return api.getUser() здесь будет работать?

Сама асинхронная функция ничего не ждёт, с ней просто работаешь, как с промисом, не путай всё в одну кучу)
Насчёт возвращения промиса из api в целом довольно логично, но в таком случае давай везде сделаем консистентно

Ну я уже не уверен что не ждёт, не удивлюсь что и ждёт после всего что прочитал, хотя понимаю что скорей всего нет. Буду потом в спецификации смотреть

Этот момент надо отражать в кодстайле, он довольно неоднозначный. Использовать await только для реальной необходимости. Использовать async если нужно вернуть промис. Даже сложно сразу внятно сформулировать, о чём писать в кодстайле

Хотя сами слова понятные: await — ждёт, async — возвращает промис

Хотя... Это всё как раз это правило и регулирует — no-return-await, может и писать ничего не надо. Просто указать что код как выше писать не надо (ждать и присваивать в переменную а потом её же возвращать)

kotosha-real · 2021-09-21T18:37:05Z

src/store/reducers/userReducers.ts

-        state = { ...state, ...action.payload };
+        Object.assign(state, action.payload);


Хотя в тулките так можно работать, там же immer

kotosha-real · 2021-09-21T18:38:35Z

src/store/reducers/userReducers.ts

-        state = { ...state, ...action.payload };
+        Object.assign(state, action.payload);


Что касается Object.assign, то лучше спреды юзать. Стильно, модно, молодёжно и поведение супер ожидаемое

kotosha-real · 2021-09-21T18:39:45Z

src/store/reducers/userReducers.ts

      })
      .addCase(updateAvatarRequest.fulfilled, (state, action) => {
        state.avatar = action.payload.avatar;
+      })
+      .addCase(userRequest.fulfilled, (state, action) => {
+        Object.assign(state, action.payload);


❓
А какие данные тут подсовываются в стейт? Хочется поменьше Object.assign

Юзер ложится в ветку стэйта user. Надо определится с конвенциями, мутирующий код мне показался более удобным (не зря он в тулките включен). assign для мутирования, через спред я не уверен как делать мутацию. Ещё аспект единообразие (если оно нужно здесь) — либо везде мутирование, либо везде иммутабельность

Не, давай на мутировании остановимся, просто Object.assign не самый лучший вариант для этого. Он работает, но я провёл быстрый соцопрос в офисе и некоторые ребята не знали, что первый аргумент мутируется

Ну спредом насколько я понимаю мутирования не сделать, хотя как по мне тут "чистота конвенций" не стоит того, и можно было бы использовать return {...action.payload}. Т.е. для мутирования объекта не так уж много вариантов помимо assign

Ну, тут согласен, все поля менять руками — то ещё удовольствие. Ты выше писал про ошибку, ты с ней разобрался? Всё ок работает?

В целом по этому таску ошибок нет, по стэйту я разобрался (мутация либо return, но return пока не проверял), по самому oauth всё работает, ошибка была при попытке oauth будучи уже залогиненным

kotosha-real

🕵🏻‍♂️

task-99: feat: внедрить OAuth

3e48615

andreyvolokitin requested a review from DariaZherebtsova September 20, 2021 18:03

andreyvolokitin added 2 commits September 21, 2021 00:31

task-99: fix: сбросить location

a989cfa

task-99: feat: использовать хук для типовых асинхронных действий

29aff2f

andreyvolokitin requested a review from kotosha-real September 20, 2021 22:45

DariaZherebtsova reviewed Sep 21, 2021

View reviewed changes

DariaZherebtsova approved these changes Sep 21, 2021

View reviewed changes

task-99: docs: добавить докуметацию для useProgress

7bf9877

andreyvolokitin mentioned this pull request Sep 21, 2021

task-100: добавить api для лидерборда #110

Merged

kotosha-real reviewed Sep 21, 2021

View reviewed changes

andreyvolokitin added 2 commits September 23, 2021 02:09

task-99: fix: рефакторить return await

cd154de

task-99: feat: передавать зависимости useCallback в useProgress

f0c3917

kotosha-real approved these changes Sep 22, 2021

View reviewed changes

andreyvolokitin added 2 commits September 23, 2021 03:00

Merge branch 'dev' into task-99

118eef8

task-99: fix: отрефакторить

bcca4cf

andreyvolokitin merged commit cc81af9 into dev Sep 22, 2021

		state = { ...state, ...action.payload };
		Object.assign(state, action.payload);

		@@ -0,0 +1,23 @@
		import { useCallback, useState } from 'react';

		export default function useProgress(

task-99: feat: внедрить OAuth #108

task-99: feat: внедрить OAuth #108

Conversation

andreyvolokitin commented Sep 20, 2021

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

kotosha-real 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

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

kotosha-real left a comment

Choose a reason for hiding this comment