#Review tednaaa/paint

[azinit]

Repository: https://github.com/tednaaa/paint

Methodology version: 2.0

[azinit]

Так-с. Таки нашел время на проект, @tednaaa готовься))

[azinit]

🟩 Начну с хорошего

praise: Очень радует, что в целом структура оч валидна, сразу плюс к +100000 соц.рейтингу FSD 😏

[azinit]

praise: И в целом, если "окунаться вглубь", то тоже конвенциональность некая прослеживается, что значительно упрощает понимание

[azinit]

praise: Прям образцовая апишка вышла, прям как и предполагалось)

С четким разделением типов ресурсов

nitpick(public-api): Но в то же время странно, что нет файла-реэкспорта и у нас прям в пути проявляется shared/api/rest/image/image.ts

[azinit]

вот тут тоже классный пример, что инстансы для API создаются в shared/api

Особенно если потом понадобится это инициализировать поптом на уровне выше

[azinit]

praise: Тоже выглядит достаточно унифицированно, особенно учитывая, что color действительно является ключевой абстракцией в приложении

[azinit]

praise: C одной стороны - интересно, что сразу относишь к ассетам шрифты

suggestion(to-improve): А в целом, кажется, что можно попробовать "полуконвенциональный" формат, с папкой app/styles, где помимо стилей можно хранить еще ассеты для этих стилей

thoughts: app, конечно, толком не регламентируется - но кажется это может упростить структуру

[azinit]

🟥 Теперь по критам

issue(public-api): Прям ключевое что проявляется прям везде - это полное отсутствие декларации Public API и хотя бы минимально необходимых файлов реэкспортов

Про линтер в конце еще скажу 😏

Хотя базовые и есть

nitpick(public-api): Поэтому для большей устойчивости модулям и более удобного использования - можно объявить файлы реэкспорты в заявленных местах
(хотя бы на уровне слайсов и сегментов)

[azinit]

Вот тут еще примеры наглядные)

[azinit]

А тут еще в целом непонятно - зачем рутовый компонент страницы выносить в canvas-page, если можно сделать общий ui/index.tsx 🤔

[azinit]

вот тут прям хорошо - при всех вопросах к неймингу внутри сегментов, как минимум есть файл реэкспорта, который помогает устойчивости и использованию

чего не скажешь про это

[azinit]

issue(layers): Прям оч пухлая страница получается, даже несмотря на то, что это основная и единственная страница приложухи

[azinit]

suggestion(layers): Как вариант - тут вот это прям просится, для выделения хотя бы в одну фичу
(если не группы фич)

Не исключено даже, что тула это еще одна сущность, но мне кажется надо подождать развития сервиса - т.к. пока звучит чуть упорото 🤔

[azinit]

suggestion(layers): Еще как вариант - часть упомянутых компонентов в pages/canvas/ui/** вынести в полноценные виджеты

Если не ради переиспользования, то хотя бы для упрощения логики страницы и поддерживания самих виджетов
(особенно если они слишком сильно подвязываются на страницу, можно плюс вместе с этим забрать часть логики модели/либ, если это будет в виджете отдельном)

[azinit]

issue(model, ui): Прям типичная проблема - что очень сложно определить, где логика модели, а где ui, и что вообще такое lib
(иногда прям все в компоненте накидывают тупо, но у тебя есть хотя бы базовое разделение - уже хорошо)

Но тут мне кажется проблемы не совсем верного определения сегментов для такой логики - т.к. это явно логика "модели", т.е. бизнес логика и дата-флоу модуля

А в lib же мы обычно заносим чисто утилитарную логику, которая НЕ относится напрямую к UI-логике или дата-флоу
(т.е. туда можно складировать что-то общее, на что может подвязаться ui/model/api или другой модуль, но напрямую lib про них знать не должен, и тем более забирать у них их ключевую зону ответственности)

[azinit]

suggestion(model): Вот тут кста прям наглядный пример-предложение как с таким можно поступать, без упарываний
(ты в целом в верном направлении мыслил)

feature-sliced/cra-template#9 (comment)

[azinit]

thoughts: Вот тут например можно прям сделать хук с useState + handle и вынести в модель, но некрит

[azinit]

nitpick: Еще пример
С одной стороны - canvas действительно относится к UI, и это как раз хороший пример, когда у нас UI не только компонентами един

А с другой - тут прям видно, что происходит оч много манипуляций с данными - что явно можно снова вынести в хук и в модель

> вся дата-логика будет в модели
> а ui останется простым, к чему мы и стремимся

[azinit]

issue(entities, features): Тут вот верно с какой-то стороны, что сущностью определен color
(color-picker не совсем про это, но не суть)

Но фичи же - это как раз то ЧТО МЫ МОЖЕМ ДЕЛАТЬ С СУЩНОСТЬЮ

Т.е. в таком случае у нас color-picker больше походит на фичу, чем на часть сущности

[azinit]

thoughts: Т.е. если у нас еще будут еще юзкейсы, завязанные на цвет - то это + еще фичи, а entities будет унифицированно отдельно лежать

С другой стороны... Если щас color только в color-picker используется, то может не стоит торопиться так с выводами, и оставить color-picker как фичу, чтобы не было излишне раздроблено
(хотя тут скорее в целом нет до конца уверенности, что color это сущность точно должна быть)

[azinit]

issue(lib): Любимая тема @sergeysova ))

TLDR: Либы (особенно если говорим про shared), должны проектироваться так, чтобы их можно было рассматривать как отдельные полноценные и самостоятельные решения, покрывающие определенный скоуп проблемы, и которые можно отдельно протестировать

Более детально, можно прочитать тут:
https://sova.dev/ru/why-utils-and-helpers-is-a-dump/

[azinit]

suggestion(lib): Конкретно тут - это то, что generateSymbols - это не либа)
random.ts, string.ts, number.ts - еще да, но не первое

Может там пока и одна функция, но это оч редко когда повод точно так же называть либу, и уж тем более хранить по либе на каждую мини-функцию

[azinit]

suggestion(lib): Тут же в цеолм сомнения, что это относится напрямую к useImageId а не условному usePathname (который в целом есть сомнения, что стоит выносить ради такого)

С другой стороны, если же хочется это унифицировать именно для imageId - то стоит такое хранить в либе рядом с местом использования
(чаще всего для этого всегда еще сущность отдельная находится - но кажется не совсем наш случай)

[azinit]

suggestion(lib): Тут прям хороший пример хука для либы, но опять же - нельзя выделить либу hooks

То что тут на скрине, это скорее либа forms.ts, или же даже положить рядом с условным инпутом в shared/ui

[azinit]

🟨 Теперь non-blocking (близки к концу уже :D)

nitpick(structure, kebab-case, non-blocking): Очень большой спор, что лучше PascalCase, kebab-case, полное отключение FSD в РФ

Но как уже говорил до этого в другом дискашне - кажется, что если выбираешь один способ описывания структуры, то лучше придерживаться его везде

Чем проще - тем лучше

Если же по тем или иным причинам начинаются встречаться в структуре в наименовании разные варианты - то это оч быстро приведет к хаосу в проекте в этом аспекте (см. "теория разбитых окон")

Тут наглядные примеры - где желательно пофиксить
(много где kebab-case, но там где красное - прям выбивается из общей массы)

[azinit]

nitpick(non-blocking): Тут прям сомнительно, что такие типы надо хранить на уровне app

Это ведь не только там используется, поэтому лучше хранить сразу где-то в src/***.d.ts файлах

[azinit]

nitpick(non-blocking): Тут опять же сомнительно, что стоит роуты прям в shared/router хранить
(даже линтер ругается :D), но прям назвать это критичным сложно

Лучше просто еще раз оставлю эту статью

https://feature-sliced.design/docs/concepts/issues/routes

[azinit]

nitpick(non-blocking, shared-config): тут не столько слайсы, сколько то, что в shared/config неплохо прорабатывать как раз не только "распаковку" значений из ENV, но и добавлять тут дефолтные значения и/или подстраховку на случай если переменная не найдена

Можно глянуть это как один из вариантов
https://github.com/ani-team/github-client/blob/workshop/feature-sliced-next/src/shared/config/index.ts

[azinit]

nitpick(non-blocking): Первое - про конвенциональность нейминга

Если хотим придерживаться какого-то +- стандарта, то лучше назвать директорию такую shared/assets (как ты уже делал), либо же сразу положить это в shared/ui

[azinit]

Другой момент - это насколько нужно хранить иконки унифицированно в одном shared

Оч часто хватает того, чтобы иконки класть с местом использования, но чтобы не размусоливать - лучше оставлю тут тред

https://t.me/feature_sliced/1742

[azinit]

nitpick(non-blocking, model): Лучше логику связанную с моделью - хранить прям в модели
Когда что-то описано в компоненте страницы, что-то в другом компоненте - прям проблемно становится это изучать и находить

Т.е. лучше эти вотчи опложить ближе к модели страницы

Ну и в контексте эффектора "вотчить" не совсем бест практис

[azinit]

Тут же прям модель похожа на ту, что описывает методология, да)
(только ругается на либу из-за отсутствия public-api)

[tednaaa]

Я просто думал что если мы юзаем другой сегмент из того же слайса, то не нужно публик апи =)

[azinit]

Ну если выделять по критичности,то скорее так:
(от наиболее важного, к наименее)

Public API:
=> component/store reexport (general reexport)
=> slice reexport
=> segment reexport
=> inner impl module reexport

[azinit]

🔳 Теперь чисто мысли

thoughts: Тут надеюсь не хочется создать "гиперконтейнер, который возьмет на себя всю ответственность по работе с данными что только можно", а чисто компонент для отображения))

[azinit]

thoughts(canvas, ui): Тут как уже говорил - canvas вроде как история чисто про UI

Однако тут видно, что есть обработчики и базовая логика переключения - что действительно лучше в модели смотрится

выглядит в целом норм, просто немного флешбечит, что сильно много специфики UI в модели

[azinit]

Хотя мб @GhostMayor @Postamentovich будет что тут сказать по этому поводу 🤔

[azinit]

FINAL

ФУХ! Боюсь смотреть сколько получилось сообщух, но постарался все основное расписать)

Спасибо за проект для ревью, постарался где мог сгруппировать схожие проблемы -надеюсь облегчит восприятие

[azinit]

@tednaaa вот тут так же предлагаю принять участие в "закрытом" бета-тесте нашего еслинт конфига

Многие варнинги со скринов - как раз на основании его выводились)

Что странно, прям не было никаких проблем в плане нарушения правил импортов между слоями/слайсами, но возможно потому что их немного

https://www.npmjs.com/package/@feature-sliced/eslint-config
https://www.npmjs.com/package/@feature-sliced/eslint-plugin-messages

[azinit]

@tednaaa Было бы круто довести это все дело до полноценного примера FSD, дай знать если захочется)