Skip to content

Conversation

@vissh
Copy link
Contributor

@vissh vissh commented Nov 25, 2023

Background

I have developed an extension using FSD architecture, I think my example and solutions will be useful. The code is located at /packages/front.

Changelog

Added in /pages/examples/img/vkaudiopad.png
Added in /pages/examples/_config.ts

@netlify
Copy link

netlify bot commented Nov 25, 2023

Deploy Preview for pr-fsd ready!

Name Link
🔨 Latest commit db161ca
🔍 Latest deploy log https://app.netlify.com/sites/pr-fsd/deploys/6562240f4fee5d000802339d
😎 Deploy Preview https://deploy-preview-643--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! The structure of the project is brilliant, I love it. One thing I'm concerned with is the set of entities.

active-track, track, and album are great entities in the business domain of your application, but card, snackbar, and content-tab — less so. They sound like generic UI components to me, and as such, I would expect them to be in shared/ui, or, if they're not generic, have a more specific name. Could you comment on that, please?

@vissh
Copy link
Contributor Author

vissh commented Nov 25, 2023

@illright hi!
Thank you for review, it has allowed me to look at my structure differently and I can now clearly see why you had questions about it.

card is as much a business entity as track and album, it's a variation of another playlist card, I've renamed it recommended-playlist.

About snackbar and content-tab I agree, they don't look like business entities. Instead of snackbar I chose the name notification, content-tab partially moved to widgets/navigation and created the active-tab entity.

Looking at everything again, I removed the controls entity because it's not a business entity at all, but parts of several features.

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.

Thanks for clarifying and making edits :)

suggestion (non-blocking): how about taking it one step further? active-tab still doesn't feel like a business entity to me, it feels about the same as current-url or something else related to navigation. However, looking at its code and its usage, I do see the problem in just putting it into shared — it knows what pages there are. Still I think that Shared is semantically a more appropriate place for navigation than Entities. I'd approach this in one of two ways:

  • Remove the knowledge about pages from the current-tab atom. Have it accept an arbitrary string in openPage and have it receive a list of valid pages on the App layer. This would hurt the type-safety of the code a bit
  • Just don't bother with this and put it into Shared exactly as it is. Yes, it knows about pages, well, so what. At least its API is ergonomic and type-safe. It's not like it's forbidden to put things in Shared that know even a tiny bit about the application, such a restriction would be way too impractical.

I would also consider putting notification into Shared as well, but that's not a big deal. Feel free to leave your code as it is, it's already a great example of FSD.

@illright illright merged commit 5339514 into feature-sliced:master Nov 26, 2023
@vissh
Copy link
Contributor Author

vissh commented Nov 26, 2023

Thanks! Your comments were helpful and I feel like I now have a better understanding of FSD.

With active-tab, I was really confused because it knows too much, so I was leaving it in entities, but I realized that wasn't quite right, and I didn't like other options either. But after moving most of it to widgets yesterday and getting your comments today, moving it to shared doesn't seem so bad anymore. I think eventually I'll come back to this issue and maybe figure out a different way to interact with the tabs.

I moved the notification to shared, that's where it belongs :)

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