Circular dependencies in JavaScripta.k.a. Coding is not a rock-paper-scissors game

Coding is not a rock-paper-scissors game and it is definitely not a rock-paper-scissors-lizard-Spock game.

You can not create random references everywhere and sleep peacefully.

It is known.

The more references you make inside your code, the harder it is to test and more complicated to understand — you end up with spaghetti code. Additionally, many references create circular dependencies, which cause bugs directly.

An obvious bug occurs when two AMD modules try to load each other synchronously:

// a.js
define( [ ‘b’ ], function() {
    function a() {
        b();
        console.log( ‘a’ );
    };
    return a;
} );

// b.js
define( [ ‘a’ ], function() {
    function b() {
        a();
        console.log( ‘b’ );
    };
    return b;
} );

You can say that ECMAScript 6 modules will handle it. That is true. This should work just fine:

// a.js
import b from ‘b.js’;
export default function a() {
    b();
    console.log( ‘a’ );
}

// b.js
import a from ‘a.js’;
export default function b() {
    a();
    console.log( ‘b’ );
}

Unfortunately, this code is not yet supported and we need to use transpilers and transpilers are tools, and tools are never perfect. In case of transpiling ES6 modules to AMD they work fine as long as you have no synchronous requirements. Unfortunately, one synchronous requirement is enough to give birth to an ugly bug.

But your case is different! You write beautiful object oriented ES6 code. You create classes. You do not have any synchronous requirements. Right?

Wrong!

When one class extends another the base class is indispensable from the very beginning. It means that the following code is enough to have a bug:

// a.js
import B from ‘b.js’;
export default class A {
    get b() {
        return new B();
    }
}

// b.js
import A from ‘a.js’;
export default class B extends A {}

This bug is ugly because if in your C module you import B module it works fine, but if you import A you get:

Uncaught RangeError: Maximum call stack size exceeded

It takes some time to realize that if you require B everything is fine. B synchronously requires A, while A does not need B in the same manner.

But if you require A, it synchronously requires B (just because it can). In such case B can not require A synchronously, because it would cause circular dependency and try to load A asynchronously, while A is needed synchronously to create B, as a base class, so we get an ugly error.

In this case, it seems to be pretty simple, but trust me, if you have a chain of 7 complex classes it is not that simple anymore. Also, you can not expect other developers to know which class they should import first.

Of course, this could be solved. The transpiler could understand what should be the proper order of loading classes, but as I mentioned earlier, tools are never perfect.

We agreed that coding is not a rock-paper-scissors game. Unless you are coding a rock-paper-scissors game. Then you may want to have such nice classes:

import Scissor from ‘Scissor.js’;
export default class Rock {
    isStrongerThan( other ) {
        return other instanceof Scissor;
    }
}

import Rock from ‘Rock.js’;
export default class Paper {
    isStrongerThan( other ) {
        return other instanceof Rock;
    }
}

import Paper from ‘Paper.js’;
export default class Scissor {
    isStrongerThan( other ) {
        return other instanceof Paper;
    }
}

This creates a circular dependency. What can we do to avoid it? We could put all classes into one file, but putting everything into one file does not seem to be a real solution. We could also create a type property and check it instead of checking instanceof. It would solve circular imports, but only because JavaScript is dynamically typed. other has to be an instance of a certain class, even if we check it in any other way. There is no good reason to create such workarounds here since this code has no synchronous references — it works fine and will not cause any errors.

So when a circular dependency is fine and when it is not?

Assume that you have a Game class. Do you think that the Rock should know that it is only a part of a bigger game? Or, if the rock-paper-scissors game is only one of many games in your system, should RPSGame has reference to the `GamesRepository` class? I do not think so. Also, if you create an Item class, which is the base for Rock, Paper and Scissor, it should not have references to these classes.

In fact, it is not a matter of circular dependencies. It is a matter of layers. There might be circular dependencies inside one layer, like in this case. There is nothing wrong with them. The goal is to create the application in such a way that layers are as thin as possible.

We had this problem during the development of CKEditor 5.

ckeditor5-engine> madge — format es6 — circular src/
view/node -> view/position -> view/text
model/position -> model/documentfragment -> model/nodelist -> model/character -> model/node
model/position -> model/element -> model/range
model/element -> model/range -> model/treewalker
model/position -> model/element -> model/range -> model/treewalker
model/operation/removeoperation -> model/operation/reinsertoperation
model/delta/mergedelta -> model/delta/splitdelt
model/delta/unwrapdelta -> model/delta/wrapdelt

You can imagine how big of a surprise was finding out that model/character being a child of model/node causes an error when we import model/position.

At first, it was unclear what went wrong with the architecture. Should we hack the code to avoid references? Using type instead of instanceof in more than 100 classes could cause even more issues.

Then we realized that (fortunately) all dependencies are always inside a single directory. That is because our architecture is not that bad after all and we have nice layers, for instance:

• utils,
• engine/model,
• engine/conversion,
• engine/controllers,
• core,
• features.

Modules from the following layer can require anything from all the previous layers, but not vice versa.

All we had to do was to split model into “structure” and “selection” layers as well. It means that Node, Element, DocumentFragment, etc. can not require Position, Range, TreeWalker etc. We had to remove methods like node.getPositionBefore, because Node does not know anything about the existence of positions. Nodes build a structure and then, on top of this structure, positions are implemented. You have to call Position.createBeforeNode instead. It makes logical sense.

After splitting classes into “structure” and “selection” layers we end up with only 3 circular dependencies:

ckeditor5-engine> madge — format es6 — circular src/
model/operation/removeoperation -> model/operation/reinsertoperation
model/delta/mergedelta -> model/delta/splitdelta
model/delta/unwrapdelta -> model/delta/wrapdelta

These are fine and do not cause any issues.

Summing up, you need layers — one will always be on top and beat the rest. The poor little one on the lower layer should not even be aware of the ones above him. Only then, it is fine to have references within one layer (even circular ones) as long as the layer does not grow too big.

P.S. If you even wonder why paper beats rock that is because education beats violence. Brain beats brawn. Mind blown!