About the intention behind S7197

The rule S7197 says the following:

This rule reports circular dependencies between source files, including indirect cycles spanning multiple files caused by circular imports.

It triggers with this code sample:

// order.js
import { Customer } from './customer.js';
import { Product } from './product.js';

export class Order {
    constructor() {
        this.customer = new Customer();
        this.products = [];
    }
}

// customer.js
import { Order } from './order.js';

export class Customer {
    constructor() {
        this.orders = [];
    }
}

// product.js
import { Order } from './order.js';

export class Product {
    constructor() {
        this.orders = [];
    }
}

And is fixed by this compliant code sample:

// order.js
import { Customer } from './customer.js';
import { Product } from './product.js';

export class Order {
    constructor() {
        this.customer = new Customer();
        this.products = [];
    }
}

// customer.js
export class Customer { }

// product.js
export class Product { }

// orderService.js
export function getOrdersByCustomer(customer) {
    // Implementation to get orders by customer
}

export function getOrdersByProduct(product) {
    // Implementation to get orders by product
}

Why is the Orders By Product relationship extracted to a service, and not the also existing Products by Order one? Even the Customer By Order relationship could be extracted. Why not? Where do you draw the line?

I believe you can’t provide architecture rules without also providing the reason why they exist. People that are comfortable with entity-relationship model are also comfortable with implementing them - they always use the repository pattern anyway and would never trigger the rule to begin with. You rule targets people that are not comfortable with implementing ER models. You can’t just feed them with arbitrary solutions without explaining the reasoning.

What about the Active Record pattern?

The rule will definitely makes codes built around the Active Record pattern explode. I’m not saying this is a bad thing - Active Record should never have existed to begin with - but you can’t ignore the fact that a lot or ORMs are built around entity classes that also act as repositories - and thus provide methods like Product::getOrders.

Typically, TypeORM:

https://github.com/typeorm/typeorm/blob/master/docs/many-to-many-relations.md#bi-directional-relations

This is their official documentation and it would trigger the rule, and their is no alternative with such a pattern.

Will you officially recommend to not use such a pattern? There are other alternatives - even with TypeORM by using entity schemas instead of decorated classes: will you officially recommend them?

Don’t get me wrong: I would love that. I - and I’m not alone - have been fighting against Active Records and decorated classes usage for years, so having the official support from Sonar would make my life much easier. But, if you do so, you need to be prepared for the shitstorm: the defense force is vindictive.

1 Like

Hi @Philippe_Formulain,

Where do you draw the line?

As the rule description says, it will report “when two or more source files import each other, either directly or indirectly”

I believe you can’t provide architecture rules without also providing the reason why they exist. […] You can’t just feed them with arbitrary solutions without explaining the reasoning.

The rule description provides the following reasoning “This creates a dependency structure that lacks a clear hierarchy, making the codebase harder to understand and maintain.”

More generally, Sonar users can chose to remove rules from their Quality Profile if the rules don’t work well for their case, or “Accept” individual issues if that is a better trade off for them. Finally, we sometimes include exceptions in rule implementations if we get significant feedback from telemetry to justify it.

Thanks for answering @gab.

As the rule description says, it will report “when two or more source files import each other, either directly or indirectly”

This is my point: what is the benefit of such a rule?

The explanation is super generic.

This creates a dependency structure that lacks a clear hierarchy, making the codebase harder to understand and maintain.

What does “hierarchy” even mean? When we design a conceptual model, the hierarchy is driven by the model, not by the project structure.

This model is legit, and comes from the conceptual model designed during the business analysis phase - which drives everything else.

  • order.ts
import type {Customer} from "./customer";

export interface Order {
   readonly customer: Customer;
}
  • customer.ts
import type {Order} from "./order";

export interface Customer {
   readonly orders: Array<Order>;
}

How this model is instanciated is out of the scope of the model itself, and it is very likely that there exist some factories for Order and Customer at the repository level that returns instances where customer and orders are lazily executed by getters, or something. But that’s not the point.

The point is that the rule would consider the model as non compliant.

Question is: why?

  • It doesn’t satisfy your “This creates a dependency structure that lacks a clear hierarchy, making the codebase harder to understand and maintain” argument
  • It doesn’t satisfy this other - arguably fallacious - argument of the rule documentation “Additionally, the order in which circular imports are resolved is not guaranteed, which can lead to unpredictable behavior and runtime errors”; interfaces don’t exist at runtime, they are not even part of the compiled artifact, how can the order of imports of something that does not exist matter?

Not sure how to help you beyond what I already shared.

I’ll let others share their views if this more of an open discussion.

I don’t need help.

I need to understand “the intention behind S7197”.

Juste saying “circular dependencies are bad” doesn’t bring any information. We can say the same about basically everything.

Also note that the argument “the order in which circular imports are resolved is not guaranteed” is plain wrong. Except if you have example, or proof, that demonstrate that circular imports are not deterministic - i.e. that executing a given artifact that contains circular imports an infinite number of times is not guaranteed to always give the same result - then this is wrong, and fallacious. Said differently, this is an argument that only serves as a support to the existence of the rule.

I don’t know of any runtime specification that says that the order of imports is not deterministic.

So, why is importing circular dependencies bad? Why is the model that I shared considered by your engineers as wrong?

That’s what I need to understand.

@gab , I gave S7197 a try today and, as I suspected, it triggers on the most straightforward - and correct - conceptual model written after the specifications expressed by the business people:

import type {Order} from "./order";

export interface Customer {
   readonly orders: Array<Order>;
}
import type {Customer} from "./customer";

export interface Order {
   readonly customer: Customer;
}

You can find the detailed issue raised by SonarQube Cloud there:

I think that this should be taken seriously because this is not a False Positive. The rule actually does what it says: it detects circular imports, and it does that successfully.

The problem of the rule is its intention, its purpose.

I suspect that, in the next few weeks, you will see some issue spikes because of this rule, issues which, I insist, will not be False Positives.

1 Like

The idea of “no clear hierarchy” is that there is no module that a reader can understand without needing to understand other modules. In effect, the whole cycle is one large module.

The idea of “the order in which circular imports are resolved is not guaranteed” is that the order the top level statements are executed depends on which module in the cycle happens to be imported first. For static imports, this is deterministic for any given artifact but may change with seemingly innocuous changes elsewhere. For await import, this might truly be non-deterministic. The order might determine whether a “Cannot access before initialization” error will occur.

In this particular case, I think a good solution is to move Customer and Order into a single file. They are only interfaces so this will not result in an excessively big file, and both interfaces need to be understood together anyway.

1 Like

@jilles-sg

Your proposal is that we put the whole conceptual model into one single module, is it?

At least each part containing circular dependencies, yes.

This is so unclear rule. Why should I make a single file with all interfaces inside? If I will make it then hierarchy becomes clear? I’m not sure about that.
I can understand when rule fires on simple ‘import {}’, but why ‘import type {}’? Compiler just ignores such imports and this is simple decoration for development type checking.

1 Like

Hi Ivan,

Having circular dependencies will make it harder to adapt the code in the future.

If they are being created by types, they could live separately from both files. If they are very coupled and small you could also consider having them in the same file.

If you don’t think it is worth it, you can dismiss the issue or disable the rule altogether.

I’m not sure what you mean by ‘being created by types’. I have pretty big set of interfaces with strong parent-child usage there. Currently every standalone entity is described in its own file and it’s simple to navigate through each entity description to maintain them. What are benefits if I will remove your circular dependency in case of interface usage?
I can understand potential problems for classes or function imports, which can be resolved by using interfaces (for classes case) and more logical functions distribution.
But what are the benefits of creating a huge file with all parent-child entities description there?

Hi, I vouch for disabling this rule for type imports. Or perhaps add functionality to configure it.

I want to use this rule, but we don’t see a value in it for types. They have no runtime effect and erased when building. This is how eslint import rule works by default: eslint-plugin-import/docs/rules/no-cycle.md at main · import-js/eslint-plugin-import · GitHub

Please consider adding this usecase :folded_hands:

2 Likes

Hi @va3y ,

Welcome to the community, and thank you for your feedback!

In my opinion, even when types are erased at runtime, the source code remains the source of truth for developers; the level at which they understand and reason about the software. And so they need to understand the cycle which is a part of it. Could you please provide more details about why you don’t see value in this rule for types? A minimal example illustrating your specific case would be helpful.

You are correct that this rule has some limitations. There are scenarios - such as the visitor pattern - where cycles should not be considered architectural issues. Currently, the only way to prevent such cycles from being reported is to place the related components in a common file. As a general guideline: the more localized a cycle is, the less likely it is to be an architectural concern. In these cases, grouping the components in a single file can be a practical solution. Conversely, broader cycles that span multiple components or structural levels are more likely to indicate deeper architectural issues that may require refactoring.

Best,

Marco

2 Likes

Came here to say exactly this!

In some cases it’s hard to avoid triggering this rule when using TypeScript types, at least not without making some fairly convoluted changes that might actually make the code less readable/maintainable.

Take this example of a module that adds a reduxjs-toolkit ‘listener’:

import { createListenerMiddleware } from '@reduxjs/toolkit';
import type { RootState, AppDispatch } from './store';
import { addSomeApplicationListener } from './someApplicationListener';

export const listenerMiddleware = createListenerMiddleware();
export const startAppListening = listenerMiddleware.startListening.withTypes<
    RootState,
    AppDispatch
>();
export type AppStartListening = typeof startAppListening;

addSomeApplicationListener(startAppListening);

The above is the setup recommended by reduxjs-toolkit but it gets flagged as ‘high severity impact on maintainability’ because I am importing the type AppStartListening inside addSomeApplicationListener

I would suggest that, for TypeScript types, this issue should not be flagged as ‘high severity’ given that it cannot lead to run-time errors.

1 Like

Quoting the earlier points:

Marco Kaufmann: In my opinion, even when types are erased at runtime, the source code remains the source of truth for developers; the level at which they understand and reason about the software. And so they need to understand the cycle which is a part of it.

Matt Leathes: I would suggest that, for TypeScript types, this issue should not be flagged as ‘high severity’ given that it cannot lead to run-time errors.

I think these two perspectives highlight exactly the core issue: there are two completely different kinds of cycles being grouped together under the same rule.

import { RootState, AppDispatch } from "./store";  // Not Okay, real runtime dependency
import { type RootState, AppDispatch } from "./store"; // Not Okay, real runtime dependency, because only `RootState` is erased
import type { RootState, AppDispatch } from "./store";  // Okay, no real runtime dependency

So while I fully understand Marco’s point about developers reasoning over source code, import type simply does not participate in the same category of architectural dependency that the rule is meant to detect. That’s why Matt’s observation is accurate: treating type-only cycles as high severity doesn’t reflect any actual runtime or structural risk and ends up penalizing a safe and idiomatic TypeScript pattern.

Thank for the thoughtful feedback @alexgwolff and everyone.

It sounds to me like this is a reasonable concern.
Could you share if you are on Cloud or Server and which version?

I created a ticket for us to consider this in our backlog for the TypeScript analysis front-end :folded_hands:

1 Like