feat: detect circular dependency for services #22
Conversation
|
@thiagomini great PR! Just wanted to request 1 thing on the tests. Could we, just as a sanity check, also add a test case where there is no circular dependency, yet we still use |
|
Hey, @tolgap , do you mean to check if there's a circular dependency between the files? We can do that, but there's an existing eslint-rule for that: import-js/eslint-plugin-import#941 I've also read somewhere that checking for circular dependency consumes quite a lot of memory, so I'm not 100% convinced we should do that - do you want to add more thoughts to this discussion? |
| { | ||
| code: ` | ||
| @Module({ | ||
| imports: [CatsModule], // ⚠️ Circular-dependency detected |
There was a problem hiding this comment.
Reminder to remove the comment
@thiagomini I meant that we should not only test the postive case, but also the negative case! |
|
|
||
| [Forward references](https://docs.nestjs.com/fundamentals/circular-dependency#forward-reference) are commonly used to handle circular dependencies between services or modules. For example, if `FooService` and `BarService` depend on each other, you can use `forwardRef()` to resolve the circular dependency. However, `forwardRef()` must be a last resort, and we generally recommend changing your code to avoid circular dependencies. This rule detects usage of the `forwardRef()` method so you can keep track of what potentially needs refactoring. | ||
|
|
||
| One strategy to avoid circular dependencies between services is to make each responsible for a single use case. That way you decrease the interface and likelihood of circular dependencies. You can also avoid using services as facades for data repositories. Instead, services should be usually used to encapsulate business logic (commands). Leave query responsibilities to repositories. |
There was a problem hiding this comment.
I'd be hesitant to advise detailed alternative strategies. It could easily be interpreted as the official way to structure a Nest app, whereas it's highly dependent on the context. i.e. if it's a simple REST app, entity-based module structure is appropriate, and simply splitting splitting the service would probably suffice.
If the app's primary responsibility is transforming and loading data, transaction scripts are a great fit.
Highly complex apps with lots of business logic would again require a different approach, and DDD would probably be a better fit, but that would depend on the skills within the team too 😄
I'd suggest we drop the second paragraph and just stick to the generic "we recommend refactoring the code to avoid the circular dependency" from the first paragraph 🙂
There was a problem hiding this comment.
Yeah, I think you are right; I thought of providing some direction to users who would ask themselves, "Then what should we do?", but maybe a blog post is a better way of doing it
Fixes #18