search

feature-sliced / documentation

🍰 Architectural methodology for frontend projects
https://feature-sliced.design
MIT License
1.5k stars 159 forks source link

React Native example #657

Closed penteleichuk closed 6 months ago

penteleichuk commented 7 months ago

Background

Changelog

netlify[bot] commented 7 months ago

Deploy request for pr-fsd pending review.

Visit the deploys page to approve it

Name Link
Latest commit 1c620248a90735df5d35c26b9c5b0f631ab87a21
penteleichuk commented 6 months ago

Все настолько плохо?😁

illright commented 6 months ago

Нет :) просто у меня не было времени сесть за ноут и нормально посмотреть. Скинь, пожалуйста, ссылку на репо, так больше вероятности, что я смогу посмотреть с телефона

penteleichuk commented 6 months ago

https://github.com/penteleichuk/Moke-Smoke

illright commented 6 months ago

Немного просмотрел проект, есть несколько замечаний:

  1. Смущает сущность i18n. Интернационализация — техническая штука, а не бизнес-сущность, этой папке бы лучше переместиться на слой shared
  2. На слое screens есть общий для всех слайсов индекс, что по идее запрещено методологией.
  3. Название слоя screens нестандартное, в таких случаях хорошо бы документировать где-то в проекте, что проект следует FSD со следующими отклонениями, а затем описать отклонения и причины, по которым они были сделаны, чтоб эта информация не потерялась со временем
penteleichuk commented 6 months ago

Обновил!

penteleichuk commented 6 months ago

🌦️

illright commented 6 months ago

Ну наконец-то я добрался до этого проекта. Прости, что так долго морозил. Проект довольно крупный, это всегда радует, когда такие примеры добавляют. Выкладываю подробный фидбек:

  1. Сегмент shared/instance, на мой взгляд, не очень хорошо отражает названием, что речь про аксиос. Инстанс — очень общий термин. Предлагаю переименовать в намного более конвенциональный сегмент api.
  2. В shared/lib заприметил папки hooks и utils, за такое в чате FSD и побить могут :D. Ну а если серьезно, в lib, по-хорошему, должны быть упакованные по смыслу библиотечки (не обязательно делать их npm-пакетами) с коротким описательным названием. Вот theme — хорошее название (хоть ей, наверное, и больше в ui место). Надо бы разобрать это по местам. Понимаю, как соблазнительно создавать такие папки, но они очень быстро выходят из-под контроля, мне в utils уже сейчас сложно ориентироваться, все очень не сгруппировано по смыслу,...
  3. ...не говоря уже о запрещенных импортах из app для редаксовых хуков :) Это известная проблема для редакса + фсд, и ей решения хорошего нет, но рекомендую объявить глобально доступный тип. Так, по крайней мере, ни людей, ни тулинг не будет смущать запрещенный импорт.
  4. Смущает сущность theme по той же причине, что и сущность i18n, но тут вдобавок то, что существует shared/lib/theme. Возникает неоднозначность, куда класть код, относящийся к темизации
  5. Советую выдерживать постоянство в названиях сущностей между единственным и множественным числом. Либо так, либо иначе, а то на этой почве могут возникать непродуктивные разногласия между разработчиками
  6. Фич, конечно, очень много. Судя по названиям, они довольно атомарные, и рискну предположить, что многие из них используются только в одном месте. Тут пара предложений, по нарастанию сложности и пользы для проекта.
    1. использовать группы слайсов. Просто переместить AuthBy*, ForgotPassword, ChangePassword, EmailActivation в одну общую папку — features/auth. Им все еще нельзя друг друга импортировать, но по крайней мере они не будут мельтешить перед глазами, когда надо понять, что может делать проект.
    2. раскидать большую часть этих фич по соответствующим страницам/виджетам. В больших слайсах на pages нет ничего плохого, это наоборот полезно, потому что связанный код находится рядом, и не обременяется излишними правилами на импорты и паблик апи. Например, все эти фичи auth затолкать в соответствующие экраны. Кстати об этом
  7. А надо ли иметь разные экраны для логина и регистрации? Из слайсов страниц можно экспортировать несколько компонентов, в этом тоже ничего зазорного. Но я не реакт нейтив девелопер, так что тебе виднее. Если хочется оставить по разным слайсам, я бы предложил, опять же, хотя бы сгруппировать грубо по смыслу экранов в группу слайсов.
  8. Мне кажется, не стоит в названии каждого слайса экрана писать Screen. Особенно это не стоит делать, когда это написано не у всех слайсов (например, Practice). Такие исключения заставляют задуматься, а почему тут не написано, это случайно или намеренно, это экран или нет.

Дай знать, что думаешь насчет этого фидбека. Спасибо еще раз за хороший пример :)

penteleichuk commented 6 months ago

Спасибо за конструктивные замечание и за ревью.

  1. Тут и правда надо поменять.
  2. Чуть сгрупировал, но там по большому счету используется по всему проекту. Разделил на либы и утилиты
  3. Сделал
  4. Сделал
  5. Сделал
  6. Сделал
  7. Скорей всего можно, но будет не совсем удобно. (мое мнение)
  8. Сделал
illright commented 6 months ago

Не могу все-таки отпустить эту тему про hooks и utils. Вижу, что стало намного лучше (респект), но все равно не хотелось бы рекомендовать другим такое разделение. Я посмотрел папочку hooks и увидел, что кроме трех хуков useApp*, все остальные используются только в одном месте в проекте. Предлагаю перенести их в это место в сегмент lib. Насчет утилит — предлагаю все-таки оставить их в shared/lib, наряду с остальными папками там, и попробовать сгруппировать еще больше. Хорошими папками в shared/lib будут dates (сюда clockify, dateDiffIn, parseValidDate), intl (getCountry, getAppCurrencies, getCurrencySymbol). Остальное тоже как-то постараться сгруппировать. И главное, чтоб по имени было понятно, что сюда можно добавлять, а что нельзя. Можно даже ридмишку для этих целей написать. А messageReceived переместить по месту использования.

penteleichuk commented 6 months ago

Я думаю что исправил рекомендации по lib. Дай знать, что думаешь

illright commented 6 months ago

Да, теперь выглядит хорошо. Порезолвь, пожалуйста, конфликт ПРа и ошибки линтера, и можем вливать :)