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 example ducks-tinder #687

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

Conversation

Stepasha419a
Copy link

Background

Add new example

Deploy is using mocks and it lacks of server logic, cause server is another big project and it takes lots of time to work together with no problems, so only mocks now. I'm planning to continue working on this project (tinder)

Copy link

netlify bot commented Jun 29, 2024

👷 Deploy request for pr-fsd accepted.

Name Link
🔨 Latest commit 1fb403a
🔍 Latest deploy log https://app.netlify.com/sites/pr-fsd/deploys/668015db908d450008a07e8c

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! Could you run our linter and make sure there are no FSD violations?

npx steiger ./src

@Stepasha419a
Copy link
Author

Hi, thanks for the example! Could you run our linter and make sure there are no FSD violations?

npx steiger ./src

Thank you for mentioning it, I've solved lots of issues because of steiger.
There're just doubtful errors remaining about merging slices between layers (insignificant-slice​) and cross-imports between features (forbidden-imports​) which allow to compose hocs between each other. I've already discussed about it, and I think that it's ok :)

@illright
Copy link
Member

illright commented Aug 1, 2024

The cross-imports are kind of a blocker. Particularly because I don't think that withPublicHocs and withPrivateHocs should be features. They are used 3 and 2 times respectively, and don't contain any business logic, so to me they are only creating an unneeded dependency. I would suggest composing the HOCs in pages instead. It will involve some duplicated code, but I believe it benefits the app's architecture.

@Stepasha419a
Copy link
Author

I would suggest composing the HOCs in pages instead. It will involve some duplicated code, but I believe it benefits the app's architecture.

If you are talking about using hocs without composition in pages f.e. for every page make N hocs composition like
export default compose(WithHoc1, WithHoc2, ... WithHocN)(Page)
i think that it can be a solution, but if I want to add a new hoc I would have to add this hoc for every page that needs... I can forget to add it and page would be without this hoc.

I just can't figure out how to implement such a behaviour without so silly composition on the same layer)

@illright
Copy link
Member

illright commented Aug 2, 2024

I can forget to add it and page would be without this hoc

I don't think that's very probable because your pages will be missing functionality of these HOCs, which will probably be caught in testing or even type checking.

@Stepasha419a
Copy link
Author

Stepasha419a commented Aug 4, 2024

I fixed these cross-import issues via routing composition like wrapping in "hoc'ed" component neccessary pages/widgets etc...
Hope it will work as it worked before)
upd: there are still remaining such steiger errors like excessive-slicing​ and insignificant-slice​ but I think that they are not so important in terms of fsd

@illright
Copy link
Member

illright commented Aug 4, 2024

The excessive-slicing is kind of important — it's signaling that a lot of the features probably shouldn't be features, and should instead be placed in pages where they are used.

As for the HOCs, I'm not a big fan of making a page slice just for HOCs. It's not a page, and HOCs are not a business domain, so using slices to keep HOCs doesn't look correct to me. Why not move these compositions of HOCs to the App layer?

I also noticed that you have app/ui, where you have routing and styles. In FSD, it's unconventional for the App layer to have the UI segment, I'd suggest taking the routing and styles out of there and turning them both into their own segments on the App layer.

@Stepasha419a
Copy link
Author

The excessive-slicing is kind of important — it's signaling that a lot of the features probably shouldn't be features, and should instead be placed in pages where they are used.

I just meant that in terms of my app, where widgets are encapsulating some kinda top logic and etc)

As for the HOCs, I'm not a big fan of making a page slice just for HOCs. It's not a page, and HOCs are not a business domain, so using slices to keep HOCs doesn't look correct to me. Why not move these compositions of HOCs to the App layer?

Where to place them if App layer can't have ui segment? In terms of the current solution via router composition "hoc'ed" components are ui...

I also noticed that you have app/ui, where you have routing and styles. In FSD, it's unconventional for the App layer to have the UI segment, I'd suggest taking the routing and styles out of there and turning them both into their own segments on the App layer.

Of course, maybe I didn't check it, because when I was fixing steiger errors I put them in ui thinking that it causes an error f.e. how it was with the other layers)

@illright
Copy link
Member

Where to place them if App layer can't have ui segment? In terms of the current solution via router composition "hoc'ed" components are ui...

HOCs are not UI typically. They are components, but they are not UI. Each HOC has its own purpose, you can use that purpose to place HOCs in different segments.

The excessive-slicing is kind of important — it's signaling that a lot of the features probably shouldn't be features, and should instead be placed in pages where they are used.

I just meant that in terms of my app, where widgets are encapsulating some kinda top logic and etc)

I'm not sure I understand what you mean

@Stepasha419a
Copy link
Author

Sorry for being so late.
I was thinking about hoc's composition issue and came to creating feature-hocs right in their compositions.
What do you think about it?

@illright
Copy link
Member

Still not great, because hocCompositions is not a feature from a semantic standpoint. It's not an action or a user flow that helps users achieve something valuable, it's an implementation detail. It shouldn't be exposed.

@Stepasha419a
Copy link
Author

Stepasha419a commented Oct 13, 2024

Still not great, because hocCompositions is not a feature from a semantic standpoint

Yes, you're right. I refactored hoc composition behaviour and now components subscribe to hoc composition updates to have relevant hocs.

I add hocs to the composition in the app lib so the hocs list will be controlled from the one place. All you need is just to subscribe the component to the composition via another hoc... Too many hocs XD

UPD: of course I removed compositions to the shared and reverted feature-hocs placement

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