-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
d3931dc
to
12a618c
Compare
packages/client/src/App.tsx
Outdated
useEffect(() => { | ||
dispatch(fetchUser()) | ||
}, []) | ||
|
||
useEffect(() => { | ||
if (userData.id) { | ||
dispatch(fetchUserTheme(userData.id)) | ||
} else { | ||
dispatch(getTheme()) | ||
} | ||
}, [userData, selectedTheme]) | ||
|
||
useEffect(() => { | ||
if (userData.id) { | ||
dispatch(addUserTheme(userData.id)) | ||
} | ||
}, [userData]) |
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.
Вместе с запросом пользователя нужно всегда запрашивать его тему, поэтому удобнее инкапсулировать эту логику внутри thunk'а fetchUser
.
То есть в компоненте останется только вызвать `dispatch(fetchUser())]
} else { | ||
dispatch(toggleTheme(selectedTheme)) | ||
} | ||
setChecked(!checked) |
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.
Когда нужно вычислить новое состояние на основе предыдущего, надежнее использовать коллбек
https://beta.reactjs.org/reference/react/useState#setstate
setChecked(!checked) | |
setChecked(prevChecked => !prevChecked) |
|
||
const key = 'theme' | ||
|
||
const getThemeFromStorage = (isAuth?: boolean): SelectedTheme => |
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.
Данные о выбранной теме сохраняются в БД, поэтому нет смысла дополнительно хранить в LocalStorage
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.
А если пользователь не авторизован?
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.
Использовать дефолтную тему
reducers: {}, | ||
reducers: { | ||
getTheme: state => { | ||
state.selectedTheme = getThemeFromStorage(!!state.userData.id) |
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.
Редьюсеры должны быть чистыми функциями, единственная задача которых вычислить новое состояние на основе текущего + payload из экшена.
Любые сайд-эффекты вроде записи в LocalStorage усложняют логику и могут привести к некорректному состоянию.
Например, если вдруг не будет доступа к LocalStorage, то selectedTheme
не поменяется, хотя редьюсер отработает.
В крупных приложениях можно получить трудноуловимые баги из-за этого
setThemeToStorage(newTheme, !!state.userData.id) | ||
state.selectedTheme = newTheme | ||
}, | ||
}, | ||
extraReducers: { |
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.
Такой вариант объявления extraReducers
устарел, лучше использовать новый через builder
import SiteTheme from '../models/SiteTheme' | ||
import ThemeService from '../services/ThemeService' | ||
|
||
class ThemeAPI { |
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.
Нет смысла создавать класс, если в нем только статические методы.
Проще каждый метод объявить отдельной функцией с экспортом наружу
export const create = (...) => {...}
export const get = (...) => {...}
export const update = (...) => {..}
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.
Делали согласно раздела Добавляем свою логику - Темизация в теории, но мысль понятна
packages/server/package.json
Outdated
"cors": "2.8.5", | ||
"dotenv": "16.0.2", | ||
"eslint-config-prettier": "8.5.0", | ||
"express": "4.18.1", | ||
"http-proxy-middleware": "2.0.6", | ||
"pg": "8.8.0", | ||
"prettier": "2.7.1" | ||
"pg-hstore": "^2.3.4", | ||
"prettier": "2.7.1", |
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.
prettier
нужен только для разработки, поэтому лучше поместить в devDependencies
.
Они не будут устанавливаться при продакшн-сборке
packages/server/package.json
Outdated
@@ -12,13 +12,20 @@ | |||
"test": "jest ." | |||
}, | |||
"dependencies": { | |||
"@types/cookie-parser": "^1.4.3", |
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.
Тоже лучше унести в devDependencies
.
Еще лучше указывать точные версии (без ^
), потому что даже минорные обновления пакетов могут содержать ломающие изменения
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.
А как такое настроить для yarn? В предыдущем модуле для npm делал через .npmrc save-exact=true
, но для yarn ответа не нашел.
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.
Создать .yarnrc
и в нем добавить:
save-prefix ""
- для yarn v1
defaultSemverRangePrefix - для yarn v2+
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.
Мое первое ревью!
2781acf
to
5b3682d
Compare
Настройка темизации с бд, фикс dev-окружения SSR, передача начального состояния редакс стора с сервера на клиент