Circular dependency issue depending on import order for bundle

[ghalle]

Using the bundle format, the order in which you use import statement may affect if the code will work or not if there is a circular dependency between two files.

For example see: https://gist.github.com/powerfear/035b81ebf5db8e4e060d

The order of the import in A.js will affect if the code generated will work or not.

That is because the generated code use variable assignment and those are not hoisted unlike the variable declaration.

I see two ways to fix this.

One is to use the exported function directly instead of assigning it to a variable. An example of this is in the gist above, the last file called "IdealOutput". This would only be for function as they are hoisted to the top just like variable declaration. This would allow for all kind of "class" related circular dependency to work. I'm not sure if this break any ES6 expected behaviour. If it does perhaps this can be supported under some sort of flag?

The other would be a way for the compiler to figure out an optimal way to order the generated code. I didn't look at the source much not sure if this is realistically possible without major rework.

[stefanpenner]

Ya we have been aware of this for a bit.

utilizing http://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm for ideal layout will likely help. @eventualbuddha hinted that he may have some time later this week to explore this further.

[domenic]

Why not work toward the ideal output? The algorithm seems potentially too smart, in that it might break expected ES6 semantics.

[stefanpenner]

@domenic I believe that alg mimics the runtime graph implicitly created by imports if one used a loader. Maybe your are correct and I am missing something...

Instead of occurring at runtime the bundle format would use this algorithm to order the layout of its output.

Cycle related caveats with or without a loader exist, but I believe this approach would yield best-effort solution. I believe these forms of cycles can break with or without a loader, and it should often be considered best practice to never create them in the first place. But if they happen, or if you are porting some large existing code-base you may have to tolerate them, at least for some time.

[briandipalma]

I guess the ideal output would be the one proposed in this issue here? I'm not aware of any correct semantics problem with that output.

[caridy]

There is nothing you can do about this. This is the temporary dead zone described by the specs. The EnsureEvaluated routine described will do this:

Dependencies graph: A -> B -> C
Seem: A -> B -> C
Evaluation order: C -> B -> A

While the evaluation of C will require B to be evaluated first, and vise-versa, in which case, depending on who requires B or C first, the entry point into the EnsuareEvaluated will be different. Imagine an scenario where there is another module D, that depends on C, something like this:

import C from './c';
var c = new C();

the evaluation process will be this (according to the specs):

Dependencies graph: D -> C -> B
Seem: D -> C -> B
Evaluation order: B -> C -> D

how do you know which order to follow when requiring A or D? you don't, that's a dead zone.

[caridy]

side note: the only way to eliminate this is by avoiding initialization in the body of the modules, and instead, use the module to export primitives. in this case it means this line https://gist.github.com/powerfear/035b81ebf5db8e4e060d#file-c-js-L8 should not be used in the body of the definition.

[stefanpenner]

Eliminating initialization in the module body should absolutely be considered best practice and in such projects, the bundled format works today.

Unfortunately the bundled format can never handle arbitrary traversals, but it can support all non cycle cases, and when cycles are present a highly probable traversal path can be selected. If a dubious scenario is detected it should likely warn.

[caridy]

@stefanpenner In this particular case, I will side with the specs. If the bundle format is going to do something that will work today, but will fail tomorrow when they start using the original code without transpilation, that will be a problem IMO. I think we should respect the TDZ and fail the same way the loader will fail in the future.

[stefanpenner]

sounds reasonable, but the layout algorithm still needs to change to be aligned with that.

[eventualbuddha]

Looking at the gist, it seems that the problem here is actually caused by default exports. Default exports behave differently than named exports because they capture a snapshot of the exported value, hence the var $$c$default = $$c$$C;. If you re-write this using named exports the problem goes away:

(function() {
    "use strict";

    function $$c$$C() {
        $$b$$B.call(this);
        console.log('C');
    }

    $$c$$C.prototype = Object.create($$b$$B.prototype);
    $$c$$C.prototype.constructor = $$c$$C;

    function $$b$$B() {
        console.log('B');
    }

    $$b$$B.prototype.createC = function() {
        return new $$c$$C();
    };

    var A$$b = new $$b$$B();
    var A$$c = new $$c$$C();
}).call(this);

This is pretty much identical to your "ideal output", @powerfear.

[caridy]

@stefanpenner that's another problem :), I don't think the bundle is allowing us to specify what is the module we are interested on, hence, we don't have an entry point to the graph. Maybe the bundle format should ask for the module to expose as global, and use that module as the entry point into the graph.

[stefanpenner]

@caridy I believe it does have this information but still needs to walk accordingly.

see: https://github.com/tildeio/rsvp.js/blob/es6-mod-improvements/lib/rsvp.umd.js#L43-L49

[ghalle]

Very interesting @eventualbuddha I wasn't aware named export access would be optimised like that. It personally solve my issue. I'll leave the issue open as there seem to be some other discussion going on.

[caridy]

@stefanpenner that's the way we tell a module to expose itself, but that tells nothing to the transpiler. If the transpiler is going to shirk a module (by aggregating all its dependencies in a form of a bundle), we need a way to specify the module to expose (the entry point to the dep graph), and the namespace where to expose, so we can apply the same principle the loader will apply to organize the modules in the bundle, and expose the module exports into the specified namespace.

[stefanpenner]

@caridy ah, understood – good point.

[domenic]

+1 to some kind of entry point module. That is similar to browserify's approach. browserify entry.js creates a script that executes entry.js, in addition to including all of its dependencies.

In ES6 terms, I think transpile-modules --entry entry.js would be somewhat equivalent to producing a <script> that acts like

<module>
import "entry";
</module>

[caridy]

@domenic great analogy, hopefully @eventualbuddha can make this happen soon :)

[eventualbuddha]

I'm having trouble seeing how this is different from the current behavior. Are we talking about automatically exporting to the global as in the RSVP example @stefanpenner linked to? Or are we just talking about dependency resolution order? If the latter, then when outputting using the bundle format this is already what happens. Please clarify?

[caridy]

after a quick chat we @eventualbuddha, it seems that the way you specify the entry point today is by pointing to the file that represents the module, and it will bundle up only the dependencies of that module. (e.g.: compile-modules convert -f bundle src/a.js -o dist/a.js)

the missing part is to get the transpiler, or a formatter to also expose the module exports into a namespace, instead of requiring to have postlude module to carry on with the task, but we can sort that out later on.

[domenic]

the missing part is to get the transpiler, or a formatter to also expose the module exports into a namespace, instead of requiring to have postlude module to carry on with the task, but we can sort that out later on.

I do not think that is missing. I don't think it's a valuable feature for the bundle transformer. It could be one for a global-variables transformer, but that should be a separate package.

[caridy]

@domenic yes, we arrived to the same conclusion, @eventualbuddha confirmed it can be implemented as a resolver.

[eventualbuddha]

I'm going to close this as "Won't fix" since there's no real action needed. I believe we concluded that the transpiler works as expected in the OP.

[chadhietala]

Has anyone had the time to implement a resolver to address this?

[stefanpenner]

@chadhietala not yet, i believe @eventualbuddha is looking into it actually again.

[sandstrom]

es6-module-transpiler is excellent, and we use it today!

Just wanted to chime in to say that if a bundler could be conceived, with compile-time resolution[1] it would make the transpiler even better :smile:

[1] http://discuss.emberjs.com/t/regressions-in-embers-loading-due-to-es6-modules/6329