Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new example to v2 list #721

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

s3rxio
Copy link

@s3rxio s3rxio commented Sep 19, 2024

Changelog

New example to v2 list.
Cat Pinterest where you can look at kittens and like them (=^・ω・^=)

Copy link

netlify bot commented Sep 19, 2024

Deploy Preview for pr-fsd ready!

Name Link
🔨 Latest commit 17d1563
🔍 Latest deploy log https://app.netlify.com/sites/pr-fsd/deploys/66ec89a40e9ef50008fd7114
😎 Deploy Preview https://deploy-preview-721--pr-fsd.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for the example! There are no issues found with Steiger, great job! Looking at the project code, I have a few remarks:

  1. I often see export * from, for example, in front/src/shared/api/index.ts. It's usually bad practice for defining public APIs, because the whole point of the public API is to restrict what objects are accessible outside of a slice. Wildcard exports make the public API less dependable and more difficult to read
  2. The login modal is located in entities/user, which I found quite unexpected. I would expect to find it in a page or maybe a widget.

In general, I think that this application is small enough that having both Entities and Features is probably more distracting than beneficial to the architecture. I would suggest just having two widgets for the cat list and the auth modal, two pages, and then just the Shared layer.

@s3rxio
Copy link
Author

s3rxio commented Sep 21, 2024

Привет, @illright!
Предлагаю продолжить на русском.

По поводу замечаний:

  1. Исправлю
  2. У меня проблемы с авторизацией. Я хочу сделать открытые модального окна авторизации когда нет токена в запросе или ошибка 401 в ответе и все это на уровне Shared API, но не знаю как будет правильно это реализовать на FSD.

@illright
Copy link
Member

Я хочу сделать открытые модального окна авторизации когда нет токена в запросе или ошибка 401 в ответе и все это на уровне Shared API

Не думаю, что это будет правильно с точки зрения FSD. Слой Shared должен быть просто связующим звеном твоего приложения и внешнего мира, а такую логику, как открытие модалки по неудавшемуся запросу, я бы туда класть не стал. Более того, такая привязка может создать больше проблем в будущем, когда логика приложения будет расти. Я бы предложил решить эту проблему на стороне дизайна интерфейса — сделать четко зафиксированные триггеры для показывания этой модалки — например, нажатие на кнопку "логин" или нажатие на сердечко, когда пользователь не залогинен

@s3rxio
Copy link
Author

s3rxio commented Oct 3, 2024

Привет!
Никак не мог найти время на рефакторинг

  • Убрал слой entities и перенес все api queries в shared/api
  • Перенес логику авторизации в features/auth
  • Перенес cats-list в слой widgets

@illright
Copy link
Member

illright commented Oct 4, 2024

Привет, спасибо!

  • "Убрал слой entities и перенес все api queries в shared/api" — супер
  • "Перенес cats-list в слой widgets" — тоже круто
  • "Перенес логику авторизации в features/auth"
    Вот тут я обычно рекомендую не создавать фичу под авторизацию. Фичи — это такое уникальное место в FSD-проекте, где в одном глобальном списке должно лежать все самое важное, что стоит знать о продукте, чтоб делать осознанный контрибьюшн. Авторизация — одна из наименее уникальных штук в приложении, и более того, довольно редко изменяемая. Если она лежит в фичах, то она занимает очень ценное пространство на "мысленной карте" проекта в головах разработчиков без особо хорошей причины.

Что с этим можно сделать: вижу, что cats-list использует из этой фичи только useAuth, который, в свою очередь, использует только AuthContext. Я бы предложил перенести этот хук и этот объект контекста куда-нибудь типа shared/api/auth или shared/auth (и туда же докинуть AuthGuard), а все остальное из этой фичи (компонент AuthProvider, AuthModal) засунуть куда-нибудь повыше, может, даже вообще в App. Главная цель — чтоб этот код не мешался и не отвлекал от основной бизнес-логики приложения (лайкать котиков)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants