Комментарии 9
Пара советов:
- Обратите внимание на
useReducer. Проще хранить всё в одном объекте (все этиpageи пр. штуки, которые вы почему-то храните в ref-ах), и менять разом. - Ещё стоит пресекать race-condition. Если проект большой и важный, то без этого вообще никак. Правда это само по себе — тема для целой статьи.
- Обработку ошибок делать более централизовано.
console.error-ы раскиданные по коду это плохо. И вообще хорошо бы уметь возвращать её (ошибку) из хука. - Кеш лучше делать опциональным (или вообще не делать), т.к. это далеко не всегда желаемое поведение.
- Ещё я бы выпилил всё что касается URL из этого хука, т.к. вы таким образом нарушаете принцип ограниченной ответственности. Такие вещи должны быть извне.
- Не увидел никакой мемоизации. Это странно для хука базового назначения.
Мы используем похожую, но кратно более сложную схему.
P.S. ещё можно посмотреть в сторону отказа от page, в пользу курсоров.
return {
currentPage: currentPage.current,
allPages: allPages.current,
}Вот тут грубая ошибка. Либо у вас это реактивный state, и тогда вы можете вернуть это из хука наружу. Либо это не реактивный стейт, и тогда render vDom древа ни в коем случае не должен от таких значений зависеть. Т.е. не используйте useRef для рективных вещей. Для этого есть useState и useReducer (ну и всякие mobX и прочие внешние сторы).
Возможно у вас сейчас этот баг никак не проявляется ввиду того, что помимо currentPage и allPages обновляется что-нибудь ещё и соответственно необходимый render всё равно происходит. Но на такие вещи точно нельзя полагаться. Это скорее из области "случайно работает".
Если вы используете useRef вместо useState только чтобы избежать лишних ререндеров — useReducer или unstable_... вам в помощь. А сейчас вы ходите по минному полю.
Ты же не думаешь, что я представил общественности первые версии хуков?) Изначально `currentPage` и `allPages` не должны были быть частью публичного интерфейса. Когда в этом возникла необходимость, я начал с `useState`. Результатом стал многократный повторный рендеринг при переключении страницы (`hasPrev` и `hasNext` также были реактивными). Потом я понял, что любое взаимодействие с хуком - это вызов `loadItems`, который влечет повторное вычисление кода хука за счет обновления состояния `items` или, в крайнем случае, `loading`.
Перечитай внимательно комментарий выше. Использование useRef для реактивных значений — грубая ошибка. Ибо это бомба замедленного действия. Тот самый скользкий тип багов, которые потом тяжело воспроизводить и выяснять причины странного поведения. Один из самых дорогостоящих видов багов для бизнеса.
Да я вижу, что смена items всё равно вызывает ререндер, но на такие вещи полагаться нельзя. Ни в коем случае. Минимальный рефакторинг в будущем, когда человек не будет иметь всей картинки костылей в голове, легко поломает этот "код". И да, текущее "случайно" работающее поведение это как раз костыль. Красный флаг.
Результатом стал многократный повторный рендеринг
Вот чтобы таких вещей не было надо вникать в то как хуки работают, какие задачи они выполняют, и каков вообще hook way в реакте. Судя по всему (по твоим ответам и коду в статье) ты пока пишешь "на ощупь". Отсюда и типовые ошибки и типовые костыли. Серьёзно, я не хочу обидеть, просто это видно издалека.
hasPrevиhasNextтакже были реактивными
Вот это тоже грубая ошибка. Которую, насколько я понял, ты уже усвоил. Тут действует простое правило — всё что можно посчитать на основе уже существующих данных — не нужно хранить в стейте. Максимум мемоизировать (useMemo), если вычисления тяжёлые. Причина банальна — ручная синхронизация = новый источник багов = дорого. В твоём случае ещё и rerender-ы.
многократный повторный рендеринг
Нет смысла, оптимизация будет преждевременной
За что боролся на то и напоролся. Когда пишешь core-вещи, т.е. обобщённый многократно переиспользуемый код (а твой хук как раз из таких), то это должна быть вылизанная до мелочей оптимизированная штука. Иначе — руки прочь из core части. Даже вопроса такого не должно возникать.
Я бы не сказал, что "хранить все в одном объекте" всегда проще
А где я сказал "всегда"? У нас на 80к строк кода всего несколько useReducer. У тебя как раз такой случай, когда useReducer упрощает понимание кода, убирает лишние рендеры, легко scale-уется в случае сложных доработок. То, что доктор прописал. Да ещё и все переменные тесно связанные между собой. Особенно если учесть что в настоящем боевом коде этот хук будет куда сложнее, когда полезут corner case-ы.
- Возможно.
Не возможно, а точно, я тебе говорю. На этапе system design такой ответ это красный флаг и "мы вам перезвоним". Тебе завтра потребуется подключить этот хук в другую часть приложения где более сложная работа с URL (или просто другая) и тебе придётся выпиливать всё до последней буквы. Да даже просто наличие на странице сразу двух постраничных виджетов (или списка списокв) и "приехали".
А причина банальная — это не задача для хука который занимается вызовом асинхр. метода который подтягивает данные согласно постраничной навигации. "low in coupling and high in cohesion" — вот главная мантра любой архитектуры. Из неё автоматически вытекает что не должно быть таких пучков которые умеют во всё сразу, особенно как-то конкретно (?page= в любой URL игнорируя рутинг приложения).
P.S. ещё можно посмотреть в сторону отказа от
page, в пользу курсоров.
Что вы имеете ввиду под "курсором"?
Когда вместо SELECT ... OFFSET {(page - 1) * limit} LIMIT {limit} используются более сложные схемы. Например берётся items.last().createdBy.toUnixTime() и возвращается в качестве курсора\якоря\как-угодно-можно-назвать. А на сервере WHERE createdBy >= {cursor}.
Это не даёт вам общего числа "страниц", зато сильно улучшает выборку данных, когда просматриваемый список элементов не статичен. Из-за постраничной навигации вы получаете дублирующие элементы или вообще теряете часть. Просто потому что между кликаниями по страницам кто-то меняет выборку.
Спасибо за ревью, друг.
1. Я бы не сказал, что "хранить все в одном объекте" всегда проще.
2. По поводу гонки условий ты (ничего, что я на ты?) прав. Добавил парочку условий.
3. Согласен, но это всего лишь пример хука, а не полноценное приложение. Думаю, что возвращение ошибки лучше оставить `fetchItems`.
4. Согласен, мне просто хотелось показать, как это можно сделать. Если в качестве `fetchItems` использовать `swr`, например, то за кеширование будет отвечать хук `useSWR`.
5. Возможно.
6. Нет смысла, оптимизация будет преждевременной.
React: разрабатываем хук для загрузки дополнительных данных