-
Notifications
You must be signed in to change notification settings - Fork 214
docs: code smells/cross-imports add #888
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
base: main
Are you sure you want to change the base?
Conversation
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
illright
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'm not so enthusiastic about the current length and focus of this page. I found it a bit difficult to read because it seemed like there were many different topics covered. Let me try to give examples:
- The first two sections describe what the article will be about, but they already go into a little bit of depth, so to me it feels like this short introduction ends up distracting from the main content. I would personally start this page from the third section, "Why is it a code smell" (with a short definition of what a cross-import is), I don't think the page would've become harder to read.
- There's a part about deep imports, which is its own problem, even outside of cross-imports. It's currently covered in the Public API reference page.
- There's an entire section about the shared layer, with examples, even though cross-imports aren't really relevant for Shared. I think this section should just be a simple clarification in the beginning.
- In the Entities section, there's a discussion about not putting UI in Entities. While it's good advice, I don't think it's relevant to the issue of cross-imports either.
- The Defining domain types in entities section gives examples of content that belongs to the Entities layer, which mostly duplicates responsibility with the Types example guide and the Layers reference
I would suggest to liberally cut this page to only leave the essentials — what we're talking about, why it's a problem, what can be done. For example, the strategies from the Features/Widgets section are nice, but they are also quite long in my opinion
Thank you so much for taking the time to provide such detailed feedback. |
|
Hi @illright, |
illright
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making these changes, I definitely think it's going in the right direction, but perhaps we overshot the destination a little bit :D
The section "Why is it a code smell" is too short now — it's essentially one sentence, preceded by an introduction to the document and followed by an introduction to the next section:
For example, when the cart slice directly depends on product UI or business logic, the domain/responsibility boundary becomes blurry.
This leaves questions in my mind — does it become blurry? why is that? and why is that bad, what is the purpose of a clear boundary between domains?
On a related note, I noticed quite a few other instances of one-sentence paragraphs — I think it'd be best to merge them into one paragraph. I like to think of paragraphs as expressing a single thought, so when a thought is spread across several paragraphs, it becomes harder for me to follow.
I also think the horizontal lines between sections are unnecessary — they strengthen the feeling of separation that is already provided by headings, which makes the page read as a collection of very loosely connected notes. Let's let the headings do the separation :)
I particularly like the "How strict you are is a team/project decision" section — it's not only very well-formatted, but also leaves no questions in my head, and yet it's concise enough to quickly read.
I also asked for feedback on this article from others in the Russian thread in Telegram because I want to check how well this content answers people's questions. Hope you don't mind :D. I'll let you know if I learn something that could help improve this article
Solant
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey 👋
Overall I agree with a point of cross-imports being bad, but I would like not to suggest using cross-import notation for the entities layer, as entities layer is a very tricky thing to do right, and cross-imports there are definitely making it harder.
Also you can mention that cross-imports prevent circular dependencies, and circular dependencies are objectively bad
Don't feel constrained by those comments, feel free to discard them if you want to restructure this article completely 😄
| ## Entities layer cross-imports | ||
|
|
||
| The `entities` layer is designed as a **domain/type-centric** layer. | ||
| It typically contains domain types, IDs, DTOs, and domain-level logic. | ||
|
|
||
| In real-world projects, entity-to-entity relationships often require sharing types or DTOs (for example, `Order` and `OrderItem`, `Artist` and `Song`). | ||
|
|
||
| To express these domain-level references, many teams use `@x` as a dedicated cross-import surface for entities. | ||
|
|
||
| For details about `@x`, see the [Public API documentation](/docs/reference/public-api). | ||
|
|
||
| For concrete examples of cross-references between business entities, see: | ||
| - [Types guide — Business entities and their cross-references](/docs/guides/examples/types#business-entities-and-their-cross-references) | ||
| - [Layers reference — Entities](/docs/reference/layers#entities) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I've seen, developers like to split everything into entities too much, even when it is not necessary, and it is the primary cause of cross-imports in entities layer (which is as bad as in other layers, frankly speaking). So maybe it is possible to mention that @x is more of a necessity than a good approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree — I also think it makes sense to frame @x not as a good approach, but rather as a necessary compromise. I’ll update the document to explicitly mention this nuance.
|
|
||
| --- | ||
|
|
||
| ### Strategy C: Orchestrate from an upper layer (pages / app) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can be called "composition", or "compose" instead of orchestration, as it is a more frequent term in frontend development.
I think you can also metion IoC (Inversion of Control) approach via render-props (react) and slots (vue) to show that it is also possible to inject components into each other to untangle them. I can help you with Vue examples if you need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the suggestion of using compose / composition instead of orchestration — that terminology feels more natural in a frontend context, so I’ll update the wording accordingly.
I also really like the idea of mentioning IoC-style approaches to untangle dependencies. I don’t have much hands-on experience with Vue projects though, so if you have time, I’d really appreciate it if you could share a Vue example when convenient. Thanks a lot!
After re-reading the article, I agree that a few parts ended up being too abstract, especially in the “Why is it a code smell” section. I’ll expand that section to better explain why the boundary becomes blurry, why that’s problematic, and what the purpose of clear domain boundaries is. Regarding the one-sentence paragraphs: I did this intentionally for readability. In Korean, longer paragraphs tend to wrap a lot, which can make the text harder to read and more tiring. That’s why I split the text. I also tried not to overdo it — rather than writing everything as one-liners, I split the content into multiple paragraphs with clear roles so it’s easier to scan and follow. I also agree about the horizontal separators — I’ll remove them and rely on headings for separation. And I actually think it’s great that you asked for feedback in the Russian Telegram thread — thank you |
Thanks for taking the time to review and share feedback. 😊 Just to clarify my intent: I didn’t mean to recommend using I realize that nuance didn’t make it into this PR, and the current wording can be read as encouraging Also, great point about circular dependencies: I’ll add a note explaining that uncontrolled or bidirectional cross-imports can lead to circular dependencies, and that circular dependencies are generally harmful. Thanks again for the feedback. |
Background
This PR adds a new documentation page that explains common code smells related to cross-imports in Feature-Sliced Design (FSD).
The page was created after reviewing real-world discussions and recurring pain points shared by developers in the official FSD Telegram community.
By researching how teams have struggled with cross-import issues, the document aims to provide a clear conceptual understanding and a set of practical strategies to avoid or refactor problematic dependency patterns.
@xas a dedicated, domain-only public API surface for cross-entity importsfeatures/widgets:entitiespages/app)