Better circular-dependencies handling

[FrancescoBorzi]

deno -v

deno: 0.1.2
v8: 7.0.247-deno

Assuming that I have two files, a.ts:

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

export class A {
    myB = new B();
}

and b.ts:

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

export class B {
    myA = new A();
}

then if I try:

deno ./a.ts

I get:

RangeError: Maximum call stack size exceeded
at eval (/path/to/b.ts, )
at DenoCompiler.eval [as _globalEval] ()
at DenoCompiler._gatherDependencies (deno/js/compiler.ts:228:10)
at moduleMetaData.deps.deps.map.dep (deno/js/compiler.ts:452:14)
at Array.map ()
at localDefine (deno/js/compiler.ts:440:34)
at eval (/path/to/a.ts, )
at DenoCompiler.eval [as _globalEval] ()
at DenoCompiler._gatherDependencies (deno/js/compiler.ts:228:10)
at moduleMetaData.deps.deps.map.dep (deno/js/compiler.ts:452:14)+

If I run the same using node (ts-node ./a.ts) it will not fail and it will execute everything correctly.

Now the question is:

Do we want deno to fail on purpose whenever there is a circular dependency?

• If no, then this is just a bug that needs to be fixed. Just ignore the text below.

• If yes, then I guess:

1. we should output an error explicitly saying that there is a circular dependency so the code cannot be executed. Otherwise it might be not that obvious for the programmer that the issue is about a circular dependency in the code.

2. we should consistently fail in all scenarios where there is a circular dependency.

For example, if I remove the new A() and new B() calls from the code, so having:

a.ts

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

export class A {
    myB: B;
}

b.ts

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

export class B {
    myA: A;
}

there is still a circular dependency, however this latter code will NOT fail to execute.

[kitsonk]

I should have written a test for this when I changed the compiler. Circular dependencies should be supported, but they are a bad idea. Of course when you add strong typing, as is in this case, they can be valid when it is a type only dependency. Even then, because while it might be an anti-pattern for some, we need to support runtime code circular dependencies.

I think the root cause is because when TypeScript attempts to resolve a module, we do almost all the work necessary to load that module, including transpiling it and evaluating it to identify its dependencies, which then causes it to depend on other modules, etc. etc. until it explodes. I think we have all the right stuff in place to fix this without a major change.

[kitsonk]

Hmmm... interesting, when I do this:

initial_module.ts

import { A } from "./modA.ts";

const a = new A();
a.log();

modA.ts

import { B } from "./modB.ts";

export class A {
  b?: B;
  log() {
    console.log("Hello!");
  }
}

modB.ts

import { A } from "./modA.ts";

export class B {
  a?: A;
}

It works. So it appears only when the initial module has a circular dependency does this fail.

[kitsonk]

Actually, correction, when it is a type only circular dependency, it works fine at the moment. It is only the runtime circular dependency that isn't working.

[kitsonk]

Ok, RequireJS (and I will assume the expected behaviour of the rest of AMD land), states this about circular dependencies:

If you define a circular dependency ("a" needs "b" and "b" needs "a"), then in this case when "b"'s module function is called, it will get an undefined value for "a". "b" can fetch "a" later after modules have been defined by using the require() method (be sure to specify require as a dependency so the right context is used to look up "a"):

//Inside b.js:
define(["require", "a"],
    function(require, a) {
        //"a" in this case will be null if "a" also asked for "b",
        //a circular dependency.
        return function(title) {
            return require("a").doSomething();
        }
    }
);

Normally you should not need to use require() to fetch a module, but instead rely on the module being passed in to the function as an argument. Circular dependencies are rare, and usually a sign that you might want to rethink the design. However, sometimes they are needed, and in that case, use require() as specified above.

If you are familiar with CommonJS modules, you could instead use exports to create an empty object for the module that is available immediately for reference by other modules. By doing this on both sides of a circular dependency, you can then safely hold on to the the other module. This only works if each module is exporting an object for the module value, not a function:

//Inside b.js:
define(function(require, exports, module) {
    //If "a" has used exports, then we have a real
    //object reference here. However, we cannot use
    //any of "a"'s properties until after "b" returns a value.
    var a = require("a");

    exports.foo = function () {
        return a.bar();
    };
});

Or, if you are using the dependency array approach, ask for the special 'exports' dependency:

//Inside b.js:
define(['a', 'exports'], function(a, exports) {
    //If "a" has used exports, then we have a real
    //object reference here. However, we cannot use
    //any of "a"'s properties until after "b" returns a value.

    exports.foo = function () {
        return a.bar();
    };
});

So, in this case, we need to return an undefined but in most cases this will cause an issue, therefore the only proper way to support it is to use dynamic import(), as we don't want people using require() in user code. The other option, as suggested above, is to just throw, since type only dependencies works, and it is only runtime dependencies that are technically unresolvable in turn.

[kitsonk]

Ok, actually looking at it more, TypeScript always uses the exports method of setting the exports, versus returning a value. So actually the internal require() isn't required, we just need to detect that we have circular reference and to return the reference to the local exports object instead of recursing.

[dharmax]

What was decided on that matter?