Circular dependencies in AMD

[Reinmar]

TODO (For me to remember) Update https://github.com/ckeditor/ckeditor5-design/wiki/AMD after closing this issue.

In some specific cases we need to deal with circular deps between our modules. We may consider switching to ES6 modules in the future (see #73), but for now we use AMD. Obviously, this is not going to work:

// foo.js
CKEDITOR.define( [ 'bar' ], function( bar ) {
    console.log( bar );
    return 'foo';
} );

// bar.js
CKEDITOR.define( [ 'foo' ], function( foo ) {
    console.log( foo );
    return 'bar';
} );

Fortunately, Require.JS allows you to require modules on demand using the require() function.

// foo.js
CKEDITOR.define( function( require ) {
    setTimeout( function() {
        console.log( require( 'bar' ) );
    } );

    return 'foo';
} );

// bar.js
CKEDITOR.define( function( require ) {
    setTimeout( function() {
        console.log( require( 'foo' ) );
    } );

    return 'bar';
} );

This will work, but how does Require.JS know that it needs to preload files foo.js and bar.js (the function, as you can see, is synchronous)? Turns out it statically analyses the code. The analysis is quite simple and fails when require() is used in some places like JS strings, in unreachable places, etc.

However, if you specify the modules that need to be loaded in the first argument of CKEDITOR.define(), then Require.JS will not analyse the code. So, if we added [] to the example above, it would fail as Require.JS would not prepare foo.js and bar.js.

Solution

We've got two proposals based on our research:

1. We should always specify the first argument of CKEDITOR.define(), even if the module has zero dependencies. That will turn off the static analysis.
2. Use postponed require() calls to get circular dependencies. Where to get require() from? There are other ways in Require.JS (like defining it as one of the deps), but in our case the simplest is to use global CKEDITOR.require() which is always accessible. So the above case should be rewritten to:

// foo.js
CKEDITOR.define( [ 'normalmodule', 'bar' ], function( NormalModule ) {
    setTimeout( function() {
        console.log( CKEDITOR.require( 'bar' ) );
    } );

    return 'foo';
} );

// bar.js
CKEDITOR.define( [ 'foo' ], function() {
    setTimeout( function() {
        console.log( CKEDITOR.require( 'foo' ) );
    } );

    return 'bar';
} );

[pjasiun]

👍

[fredck]

Turns out it statically analyses the code.

Do you have a reference for this?

[pjasiun]

Turns out it statically analyses the code.
Do you have a reference for this?

If you do:

CKEDITOR.define( function( require ) {
    console.log( "require('xxx')" );

    return 'foo';
} );

You will get an error that xxx can not be loaded.

And id you do:

CKEDITOR.define( function( require ) {
    var bar = require('b' + 'a' + 'r');

    return 'foo';
} );

bar will not be loaded.

It is a pretty simple mechanism.

[fredck]

Thanks for the clarification.

I'm trying to understand the benefit of the following:

We should always specify the first argument of CKEDITOR.define(), even if the module has zero dependencies. That will turn off the static analysis.

This one sounds irrelevant. The rule should be about always passing dependencies to define(), even if you have synchronous require() to such dependencies in the code.

[pjasiun]

If you do:

CKEDITOR.define( [], function( require ) {
    console.log( "require('xxx')" );

    return 'foo';
} );

There will be no errors because the code will not be analysed.

Also if you do:

CKEDITOR.define( [ 'bar' ], function( require ) {
    var bar = require('b' + 'a' + 'r');

    return 'foo';
} );

Everything will work fine because require.js will now what modules to load.

[] parameter is the list of modules which will be used in this module. If it is not defined the code will be analysed. And since we do not want to use the static analysis tools it is a good practice to turn it off.

[fredck]

since we do not want to use the static analysis tools it is a good practice to turn it off.

Why? Do we have any realistic problem with it?

A "good practice" would be instead not naming functions "require", which seems to be a pretty obvious thing in a AMD environment.

Forcing us to always pass dependencies even if we don't have them sounds like one of the of those over-engineering wtfs, IMHO.

[Comandeer]

I think that the problem is in the fact that AMD tries to be compatible with CJS and omitting passing dependencies in define allows AMD loader to scan function in order to find all require calls: https://github.com/amdjs/amdjs-api/wiki/AMD#simplified-commonjs-wrapping-
AFAIR it's the part of AMD standard only because the standard is based on require.js implementation. And because of that standard is just messy…

[Reinmar]

since we do not want to use the static analysis tools it is a good practice to turn it off.

Why? Do we have any realistic problem with it?

Most likely not. I guess that they tried to at least cover frequent cases like using require() in comments. But at the same time we may by mistake base on the static analysis, which perhaps isn't supported by Almond or whatever else. Having a rule that we always specify dependencies would allow us to forget about this totally. And I would like to forget about this, because this is weird :P.

[fredck]

Do we have any realistic problem with it?

Most likely not.

Well, that should tell us what to do :P

And I would like to forget about this, because this is weird :P.

Until someone go ask you "why the hell we are forced to set empty dependencies if AMD says it's optional?" (the wtfs I was talking about... after all the code will just look weird)

[Reinmar]

Well, that should tell us what to do :P

I knew from the moment I wrote the first message, that I will have to agree with that :P.

[Reinmar]

Anyway, the first proposal is officially rejected. If there are no objections about the second one I'll extend the doc about AMD.

[fredck]

Well, the second one is the only way to make circular references to work, so it's a must have and it's good to have it documented.

[pjasiun]

Do we have any realistic problem with it?

I found such case:

CKEDITOR.define( function() {
    function Foo () {
        var bar = CKEDITOR.requre( 'bar' );
    }

    return Foo;
} );

It will work as long as someone will not add modules he wants to load. For example:

CKEDITOR.define( [ 'bom' ], function( bom ) {
    function Foo () {
        var bar = CKEDITOR.requre( 'bar' );
    }

    return Foo;
} );

This code will stop working because now 'bar' is not loaded. It means the error is in the totally different place that the one was modified.

If we decide to use [] from the very begging we will see the problem when we add: CKEDITOR.requre( 'bar' ). It would need to be added to [] ([ 'bar' ]). We will see the error when the change is done.

It fact the situation is even worst. If any other plugin will load bar, the incorrect code will work. So if the test code will load var modules = bender.amd.require( 'bom', 'bar' ); we will do not know that something is wrong.

[Reinmar]

As we adopted ES6 modules this is a much smaller issue now, although Babel makes things harder: https://phabricator.babeljs.io/T6880. For now we're using a patched version of Require.JS which works around this problem, but we'll need to figure our some other way. Anyway, this topic can be closed.