Pull to refresh

Comments 9

Пара советов:


  1. Обратите внимание на useReducer. Проще хранить всё в одном объекте (все эти page и пр. штуки, которые вы почему-то храните в ref-ах), и менять разом.
  2. Ещё стоит пресекать race-condition. Если проект большой и важный, то без этого вообще никак. Правда это само по себе — тема для целой статьи.
  3. Обработку ошибок делать более централизовано. console.error-ы раскиданные по коду это плохо. И вообще хорошо бы уметь возвращать её (ошибку) из хука.
  4. Кеш лучше делать опциональным (или вообще не делать), т.к. это далеко не всегда желаемое поведение.
  5. Ещё я бы выпилил всё что касается URL из этого хука, т.к. вы таким образом нарушаете принцип ограниченной ответственности. Такие вещи должны быть извне.
  6. Не увидел никакой мемоизации. Это странно для хука базового назначения.

Мы используем похожую, но кратно более сложную схему.


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-ы.


  1. Возможно.

Не возможно, а точно, я тебе говорю. На этапе 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. Нет смысла, оптимизация будет преждевременной.

Автор статьи, уберите, пожалуйста, виджет StackBlitz под кат. Иначе он передёргивает фокус на себя и главная Хабра автоматом прокручивается на него при каждом открытии.

А лучше под <spoiler/>, чтобы не только главная не прыгала, но ещё и сама страница статьи.

Only those users with full accounts are able to leave comments. Log in, please.