Eslint-plugin-import: rule to prohibit circular dependencies

[ljharb]

I'd love a rule that errored out whenever any circular dependency exists - supporting both require and import.

[benmosher]

I think this is pretty easily achievable. Would be cool. Probably just need to enhance ExportMap to make a note if it finds its way back to itself.

re: supporting both require/import, moduleVisitor exists to abstract away the import type. so in the future we can add dynamic import() support there and all rules can pick it up immediately.

Code: https://github.com/benmosher/eslint-plugin-import/blob/master/utils/moduleVisitor.js

see no-unresolved rule implementation for an example

[aminmarashi-binary]

I need this, so I'm going to give it a shot.

[mzedeler]

See also: https://github.com/eslint/eslint/issues/9096

[benmosher]

fyi (esp. @aminmarashi) I just roughed this in on the no-cycles branch. haven't tested what I've got at all, just jotting down thoughts mostly. will post here if/when I get it completed.

[benmosher]

pushed a working version with a handful of tests to no-cycles branch. I am concerned it is super inefficient since it may BFS-search the full dependency tree for every import in every file in a project. certainly seems like some broad persistent cache would be helpful.

if anyone can test it out on a decent-sized project and post before/after lint times, that would be great info.

[benmosher]

cc @RealBlazeIt: thought you might want to know this is in progress since you suggested it to ESLint core in eslint/eslint#9096, as noted by @mzedeler

[benmosher]

First draft increased my test project lint time from 1 to 2 minutes. Pretty substantial increase. That said, also a pretty helpful feature.

I will keep thinking about cache strategies but I’ll probably go ahead and merge the rule in the mean time, since it is working and useful as is, and not as horrible perf as I had initially worried (for my ~1000 module test project, anyway).

[sibelius]

we have 2 rules to handle this already

import/no-cycle
import/no-self-import

can we close this?

[ljharb]

Indeed; closed in #1052.

[Aprillion]

thanks so much for import/no-cycle rule!

I discovered that even import {MY_CONSTANT} from './a' can be sometimes undefined instead of the static string value because of 1 cyclic import (almost like a race condition - deterministic but different in various unit tests that import the 2 files in different order) and it behaved differently on my local machine few weeks ago vs today, so really appreciated.

[jaylattice]

Our codebase uses import and require, along with module aliases. It looks like import/no-cycle does not account for both of these cases. Correct me if I'm wrong. I was considering writing a rule similar to this one that also accounts for require calls and also attempts to resolve the path to an alias, if any, before adding it to the dependency graph.

[ljharb]

@jaylattice ideally the rule should handle them all. if it doesn’t, please file an issue/PR for that :-)

[wycats]

How hard would it be to limit this rule to cycles, where one of the cycles uses another's top-level (other than function declarations), like:

// a.js

import { B } from "./b";

export class A extends B {}

// b.js

export class B {}

This is the case the triggers the "undefined" problem in transpiled output, and the one I really want!

[ljharb]

@wycats if it’s possible to statically detect “safe” cycles and not warn on them, that’s certainly an option that could be added - so far tho, the general community sentiment has been that any form of cycle should be avoided, for maintainability reasons, beyond just runtime issues. Happy to review a PR!

[wycats]

@ljharb do you know what maintainability issues people are worried about?

Personally, I find it useful to have the ability to break up large files into smaller ones, even when the functions inside of them are recursive.

[wycats]

@ljharb I realized that you can think of what I want as equivalent to a multi-file version of the { "variables": false } option in no-use-before-define.

[ljharb]

Visualizing and reasoning about the dependency graph of code - in the same file or across multiple files - is generally easier when you don't have mutually recursive implementations. I agree that if it's already designed that way, multi-file vs single-file isn't particularly different.

[rohitkrishna094]

sorry if this is the wrong place, but how do I disable this rule? I am unable to find any documentation or stackoverflow answer on how exactly to disable "Dependency Cycle Detected" -> import/no-cycle.

This link doesn't mention how to disable it.

https://github.com/benmosher/eslint-plugin-import/blob/v2.20.1/docs/rules/no-cycle.md

Thanks

[Aprillion]

@rohitkrishna094 Same as any other eslint rule - https://eslint.org/docs/2.13.1/user-guide/configuring

e.g. import {x} from './y' // eslint-disable-line import/no-cycle (in all of the affected files, not just in 1 place)

or rules: {... 'import/no-cycle': 0, ... } in .eslintrc

[rohitkrishna094]

@Aprillion Thanks for your reply. I was hoping to find a rule that I could put in .eslintrc.json and therefore it would apply to all files instead of me having to add // eslint-disable-line import/no-cycle for every place I have that error.

Thanks

[ljharb]

@rohitkrishna094 you can, just like any other eslint rule - add "import/no-cycle": "off" to your rules section.

The best solution, of course, is to get rid of the circular dependency.

[icedtoast]

Hi @ljharb - I'm hitting a case now, where I think the cycles are safe.
Basically I have:

// deserialize.js
import Apple from './apple.js';
import FruitBowl from './fruitBowl.js'

export default function(jsonObject) {
 switch(jsonObject.type) {
   case: 'apple':
      return Apple.from(jsonObject);
  case: 'fruit-bowl':
      return FruitBowl.from(jsonObject);
}

// fruitBowl.js
import deserialize from './deserialize.js';

export default class FruitBowl {
   static from(jsonObject) {
     const ret = new FruitBowl();
     ret.content = deserialize(jsonObject.content);
   }
};

I've included the 'apple' case for completeness.
So paraphrased, I have function which constructs a set of different objects via a static method on their classes, and some of those objects themselves hold other objects - so their classes also import the general deserializer.

So my questions are:

1. Is this pattern actually safe?
2. If not, how can I change it?
3. If it is safe, how can eslint detect that and not warn?

Thanks.

[ljharb]

"Safe" isn't the only nor the most important concern. Cycles are hard to think about, and that is many times more important than runtime performance or safety concerns.

What I'd suggest is, instead of deserialize possessing knowledge of Apple, FruitBowl, etc, to construct a common protocol - like Symbol.iterable, 'then', toJSON (the perfect example for serialization) etc - and have each type define on itself the serialization wrapper. That way, deserialize has zero dependencies, and each type would instead just import deserialize (and the protocol Symbol, from the same file) as needed.

[wycats]

The safest cycles are cycles between hoisted functions, which have no safety issues at all.

A totally unrealistic example:

// countdown.js
import { countdown2 } from "./countdown2";

export function countdown(count) {
  if (count === 0) {
    return;
  } else {
    console.log(count);
    countdown2(count - 1);
  }
}

// countdown2.js
import { countdown2 } from "./countdown2";

export function countdown2(count) {
  if (count === 0) {
    return;
  } else {
    console.log(count);
    countdown(count - 1);
  }
}  

This case is completely safe because the hoisted functions are initialized before any code is evaluated. A more realistic example of this sort of pattern is a tree walker that breaks apart different parts of the walk into different files (e.g. walking HTMLElement in one file, SVGElement in another file, and TextNode/Comment/etc. in a third file). Rejecting cycles for situations like this means people are forced to put all of the code in one file, or do other awkward transformations to avoid a totally natural, totally safe pattern.

Aside: another totally safe kind of cycles involves TypeScript: one of the sides of the cycle is a type. These days, you can use import type to be clear about that situation, but not everyone does, and the TypeScript mode that warns in the absence of import type allows types to be added to existing import declarations as long as at least one non-type import is already present in the import. This affects auto-import as well.

A slightly less safe, but still pretty safe pattern is cycles that arise entirely because of use inside of a function or method.

// countdown.js
import { countdown2 } from "./countdown2";

export const countdown = (count) => {
  if (count === 0) {
    return;
  } else {
    console.log(count);
    countdown2(count - 1);
  }
}

// countdown2.js
import { countdown2 } from "./countdown2";

export const countdown2 = (count) => {
  if (count === 0) {
    return;
  } else {
    console.log(count);
    countdown(count - 1);
  }
}  

This case is slightly less safe because it creates problems if either of the functions is invoked during module evaluation (at the top level). If the cycles arise because of instance methods, the cycle is safe as long as none of the classes are instantiated during module evaluation.

This sort of case arises most commonly when building a recursive data structure (instead of walking the DOM, imagine implementing a DOM-like data structure).

In both of these cases, cycles arise naturally when code for walking or implementing a cyclic data structure gets too big for one file. The original design for modules (which I helped champion) intentionally supported cycles to enable people to break up large modules into smaller modules, even when the large module originally worked with cyclic data structures.

To help address the confusion caused by cyclic modules, the JavaScript spec makes it an error to attempt to reference an uninitialized import.

In the intervening years, most implementations of modules that people have experience with (e.g. the one in webpack) do not reliably produce this very important error. In my opinion, the lack of an error for accessing uninitialized imports during a cycle has made the real danger of cycles much greater in those environments.

I've been writing JS almost exclusively with modules since roughly 2015 (ember-cli shipped module support in its first release). I totally agree that weird undefined errors caused by silently saving off an uninitialized binding is a source of massive frustration. Over the years, I have repeatedly used madge to track down silent cycles of this form.

That said, I think the claim that cycles are hard to think about broadly is throwing the baby out with the bathwater. This claim puts pressure on people to avoid breaking up modules in situations involving cyclic data structures.

To be very clear, I have no objection whatsoever to the existence of a strict no-cycles rule. I do, however, believe that it makes sense for this lint to have an option to allow cycles in some of the less dangerous scenarios, just as many other eslint lints do. I don't agree with the proposition that these sorts of safe (or mostly safe) cycles pose an unusual danger that justifies rejecting an option to allow them.

This is not the general philosophy of eslint, and I genuinely don't understand why @ljharb is taking such a strong line here.

---

@ljharb I don't agree with your analysis, but don't really want to spend the rest of my life arguing about it on this thread. I replied because I wanted to make sure the perspective in favor of certain kinds of cycles (in situations involving cyclic data structures or algorithms) was represented.

[ljharb]

eslint plugins don't need to care about the philosophy of eslint core, and this rule, like all plugin rules, is intended to be opinionated :-)

I very much agree with your analysis of safety, and appreciate the extra context. The rule is not primarily about ensuring safety.

The option to allow them is, like any rule, to disable the rule :-)

[wycats]

The rule is not primarily about ensuring safety

Just to be clear for anyone following along: my comments were not intended (primarily) to explain why certain situations are safe. They were intended to explain my perspective that certain safe situations are important, and to disagree with the un-caveated perspective that cycles are intrinsically confusing.

Of course anyone can disable the rule, but eslint rules very often have graduated options so that people can choose their own cost/benefit analysis.

Sincere thanks for explaining that eslint-plugin-import takes a more aggressive perspective, avoiding options that the maintainers believe will erode the benefit of the lint.

[ljharb]

@wycats if there's a way an option could be added that would permit "safe" cycles, in the way you describe, accompanied with very clear and convincing documentation that explains the difference between what the option allows and what it forbids, I'd be quite willing to accept that PR.

[wycats]

@ljharb would that include the mostly-safe cycles (cycles caused by references to bindings inside of functions or methods, in situations without direct top-level references to those bindings)?

[ljharb]

Since you're the one that feels strongly about making allowances, I'm content to defer to your (well-documented in the PR) judgement. Multiple option levels are also fine, if there's different categories folks might want to allow/prohibit.