jorendorff / js-loaders

Pseudoimplementation of the proposed ES6 module loaders.
54 stars 7 forks source link

Circular dependency handling for dynamic execution #102

Closed guybedford closed 10 years ago

guybedford commented 10 years ago

It is common for NodeJS modules to be created with circular dependencies.

I believe the assumption is that the first module will execute with a binding to the second before it has been populated. It has a lot of edge cases, but many modules are designed to work this way. See http://nodejs.org/api/modules.html#modules_cycles

The dynamic instantiation still hasn't been fleshed out from what I can see. If it is possible, this code should do the following:

  1. When a circular dependency is caught for a dynamic module, create an empty module object for the circular dependency, and run the execute function.
  2. When running the execute function of the other module, ensure its binding matches the original.

For example, Browserify already caters for circular dependencies. So we have module authors creating node modules for the browser based on these principles.

It would be great to catch this for maximum compatibility.

guybedford commented 10 years ago

Perhaps one way to implement this would be to have dynamic instantiation use a new exports property, and then have the execute function return a plain object:

instantiateResult = {
  deps: [],
  exports: ['export'],
  execute: function() {
    return {
      export: 'value'
    };
  }
}

This exports array would allow the algorithm to create a shell module object:

Module({
  get export() {
    EnsureEvaluated(this);
    return getEvaluationOutput(this)['export'];
  }
})

The module then only executes itself when accessed.

I may be missing something, but surely this would allow for much closer existing NodeJS module compatibility?

Update - I'm mixing this up with ideas of deferred execution, apologies. This is unnecessary for CommonJS, we simply need to use an empty shell module that can be extended later on. Having execute return a plain object is all we need for compatibility here I think.

dherman commented 10 years ago

I'm not understanding the problem yet. In the presence of cycles, an AMD or Node/Browserify shim should implement the execute method as something that pre-allocates objects and passes them into the user module functions, and when it gets an object back from the user module function it should convert that to a Module object and return that from execute. IOW, your compatibility shim has the responsibility for creating the plain objects that it shares with the user code. Does that not work for some reason? Otherwise I'm not quite getting what having execute return a plain object would mean, or how it helps. Can you elaborate?

guybedford commented 10 years ago

NodeJS handles cycles by relying on the fact that require statements execute the modules at that point in the code (http://nodejs.org/api/modules.html#modules_cycles):

If I write require('a') where we have:

a.js:

exports.export = 'export';
var b = require('b');
console.log(b.export == 'export');

b.js:

var a = require('a');
console.log(a.export == 'export');
exports.export = 'export';

I would get two true statements logged.

The exports object is built up sequentially and circular imports get the half-written export object.

AMD handles cycles by returning an undefined object when hitting a cycle. The AMD CommonJS form works similarly to NodeJS though I believe (http://requirejs.org/docs/api.html#circular).

The above that I described doesn't cater to these cases at all unfortunately, I'm still trying to work out exactly what would be required here.

Ultimately I think there are three levels of support one could provide:

  1. Don't support them at all (current)
  2. Catch them, but when caught return undefined for the circular dependency before it has been executed (what AMD does).
  3. Support creating an empty module object for a circular dependency and then allowing the execute function to populate a module object. Something like:
{
  deps: ['some', 'deps'],
  exports: ['these', 'are', 'the', 'exports'],
  execute: function(exports, normalizedNames) {
    // we now support amending the exports object
    exports.these = 'hello';
    var circular = System.get(normalizedNames[0]);
    exports.are = 'another';
    // we build up the module sequentially, just like NodeJS does
    // execute doesn't return the Module object, as the shell already exists.
  }

This would provide the full NodeJS compatibility.

Note that the last option would also need to support deferred execution of dynamic modules, so they aren't executed during linking, only during EnsureEvaluated. I think the shell module concept would work well with this, as the shell module would effectively be constructed during linking.

neonstalwart commented 10 years ago

AMD handles cycles by returning an undefined object when hitting a cycle.

i don't think that's quite the full story. for AMD, to support circular dependencies, you can request "exports" as a dependency and then it works the same way as node - the exports object is passed to fulfill the circular dependencies and the module augments that exports object rather than provide a new value. you don't have to use the CommonJS form in AMD in order to leverage this.

what @dherman describes sounds like a feasible option - the compatibility shim becomes responsible for generating the exports object and the modules need to continue to play by the same rules they have always needed to play by when there is circular dependencies.

neonstalwart commented 10 years ago

as a point of reference for using "exports", these 3 modules in dojo cause a circular dependency and each depends on "exports" but none of them use the CommonJS form

guybedford commented 10 years ago

@dherman I'm trying to understand your suggestion. My assumption is that the dependencies are loaded in the execute function by a System.get call, which would result in direct execution, at which point the question for the loader becomes what module object to use in a circular scenario.

If we assume this is handled by the user code in the execute function, how would it know if a given dependency is a dynamic or declarative module though? Or even how would it know if we are in a circular scenario? For example, it can be useful to support allowing a CommonJS module to have jquery coming from an ES6 version. I suppose I'm not clear on what this might look like.

The suggestion (3) I've made above would fully cover the circular execution scenario identical to NodeJS with the loader handling the creation of a module shell.

guybedford commented 10 years ago

It seems I have misinterpreted the instantiate function form here somewhat. I'm closing this, attempting the currently suggested method, and if necessary will post feedback elsewhere.

unscriptable commented 10 years ago

I still don't see how this could possibly work. Can somebody explain how returning an execute function and a list of dependency names could handle circular dependencies? Thanks.

guybedford commented 10 years ago

John you are exactly right - it can't, and this was my confusion as well. The solution is for AMD and CommonJS module compatibility layers would handle their own module side table to allow circular registries. I'm adapting SystemJS at the moment to use this process, which will support circular references. I'll copy you in on the code when it's working if you're interested.

unscriptable commented 10 years ago

Thanks, @guybedford. I was totally not expecting that we'd have to circumvent the ES6 loader!

It seems unfortunate that we have to waste memory and cpu time to manage an ES5 side-loader, but I can see now that a few other issues we encountered will go away when we aren't trying to make the ES6 loader handle ES5-ish things.