I've been using circular-dependency-plugin, but it's not compatible with "importsNotUsedAsValues": "preserve". When this setting is enabled circular imports that are used as types only are treated as errors, but with the setting disabled they are not. And import type cycles are never treated as errors. If a lint rule is added for this I would hope it would have configuration options about whether or not to raise errors on type-only imports.

@JoshuaKGoldberg To implement this feature, we need to preserve the state between linted files - it's needed to build the dependency graph between the modules.
create function is called once per file, so we can't keep the state there, in the closure.
It seems that the only possible way of doing that is to keep the state object outside of the object passed to the createRule function. Please have a look at the pseudo code below.

Is this an acceptable way of doing that? I wonder if there's some case where we would start linting with stale state (with some records from previous linting).

const graph = new Map(); // state - keeps track of dependencies

export default createRule({
  name: 'no-circular-dependencies',
// ....
  create(context) {
    return {
      ImportDeclaration(node): void {
         graph .set('fileName', 'importPath'); 
    }
}

@krzysztof-grzybek Your comment is actually roughly how the import/no-cycle lint rule works.

Their lint uses the AST from the current file to determine its dependencies, then it does an out-of-band parse on each dependency to determine its dependencies, and so on.

To be clear - this proposal is not to rebuild that lint rule in the same way.

Our lint rule wouldn't need to store any state because we have access to type information.

We can rely on the fact that TS has already analysed every file and knows the dependency graph of the project.
So instead of manually building it - we can just ask TS for the information.

This is the basis of this proposal - the import/no-cycle lint rule is slow because it has to parse dependencies itself but we could potentially be faster because TS has already got the information.

Is circular graph error / problem? I think I recall that it can be more a problem with CJS than ESM but I don’t find really supporting documentation.

Webpack used to warn about this but it no longer does that by default.

What I’m after here, is some good but short explanation why user would like to enable this rule and when not to use it. This will probably be filled in the opened PR but same time to help that, maybe it would be good to collect some sources :)

I tried to look the documentation of other tools. Most do not really have anything to say why you want this. Probably it’s so common knowledge that it does not need stating.

• https://www.npmjs.com/package/circular-dependency-plugin says “ Circular dependencies are often a necessity in complex software, the presence of a circular dependency doesn't always imply a bug, but in the case where you believe a bug exists, this module may help find it.”

Something similar could be added to the new rule, that it may not be a problem.

Reason that it may be problem

• runtime error (this is something that is saw, but I’m not sure if the exact circumstances why and how)
• It may prevent tree shaking while bundling (needs source, but I think webpack may bail out in this situation)

@rubiesonthesky

// fileA.ts
import { b } from './b';

export const a = b;

//  fileB.ts
import { a } from './a';

export const b = a;

What's the result of a 3rd module doing import { b } from './b';?

The tl;dr is that cycles in the dependency graph increase complexity greatly. It makes it hard to reason about code because you need to think about and understand the way in which the underlying ESM infra works.

And that's assuming your build tool / bundler transpilers your code in such a way that it works how ESM says it should.

Why take on that complexity when the vast, vast majority of cases could be refactors to remove the cycle? Moving a variable or a function to a new module can break the cycle and restore your dep graph to be an acyclic graph and reduce complexity.

@rubiesonthesky

• runtime error (this is something that is saw, but I’m not sure if the exact circumstances why and how)

1. ReferenceError will (probably) occur in Brad's example. (This is what has happened in my experience with webpack.)

2. You can also easily overflow the callstack

   // Assert.ts
   import { reportAssertion } from './Report.ts';
   
   export function assert(x) {
      if (!x) { 
           reportAssertion();
           throw new Error();
      }
   }
   
   // ReportAssertion.ts
   import { assert} from './Assert.ts';
   
   export function reportAssertion() {
       try {
          throw new Error(); // suppose the network is not available or something
       } catch {
          // should have reported
          assert(false);
       }
   }
   
    // main.ts
   
   assert(false); // <-- (if reporting crash fails) Uncaught RangeError: Maximum call stack size exceeded

   You could also do this within a file with

   function f() {
      g();
   }
   
   function g() {
       f();
   }
   
   f() // Uncaught RangeError: Maximum call stack size exceeded

   but it's a lot easier to write by accident and harder to debug across circularly imported, maybe even distantly related files

The real value IMO isn't 2-module cycles like you always see in examples. Those cases are always simple and often are easy to spot by hand.

The more interesting cases are when you've got a cycle where the path is 3 or more. I've seen cases where there are a dozen modules in the cycle - nearly impossible for a human to see.

The other example that comes to mind is

// fileA.ts
import { something } from 'fileB.ts';
console.log('side effect that needs to happen first');

// fileB.ts
import { somethingElse } from './fileThatTransitivelyImportsFileA.ts';

console.log('side effect that needs to happen second');

// main.ts
// transitively import fileA or fileB

// order of console logs can be changed by addition or removal of files in cycle 

There is of course more than one code smell here, but... it happens in the wild.