Dynamic module `execute` called with no arguments instead of Module arguments

[guybedford]

As far as I can tell, execute is mentioned in LinkDynamicModules, where it is referred to as factory (filed in the list of corrections), and called with no arguments.

But the execute method is documented as being called with the dependency modules as arguments.

Further, I also suggested this all be done at the EnsureEvaluated stage to keep with deferred execution principles.

[guybedford]

In order to make dynamic modules work, here is the LinkDynamicModules function that I am using currently -

   function LinkDynamicModules(loads, loader) {
      while(loads.length) {
        load = loads[loads.length - 1];
        load.status = 'linked';
        var depModules = [];
        for (var d in load.dependencies)
          depModules.push(loader._modules[load.dependencies[d]]);
        var module = load.execute.apply(null, depModules);
        if (!(module instanceof Module))
          throw new TypeError('Execution must define a Module instance');
        load.module = module;
        finishLoad(loader, load);
      }
  }

It would be nicer if this can be moved into EnsureEvaluated still though.

[guybedford]

The above doesn't work due to the fact that one might link a load before it's dependency.

Note that this also has to assume the fix in https://github.com/jorendorff/js-loaders/issues/85 that a linkset is only linked after any dependent linksets.

Here is the corrected function for LinkDynamicModules:

      // continue until all linked
      // note that circular dependencies will stall this loop
      while (loads.length) {
        // search through to find a load with all its dependencies linked
        search: for (var i = 0; i < loads.length; i++) {
          var load = loads[i];
          var depModules = [];
          for (var d in load.dependencies) {
            var dep = load.dependencies[d];
            // being in the module table means it is linked
            var depModule = loader._modules[dep];
            if (!depModule)
              continue search;
            depModules.push(depModule);
          }

          // all dependencies linked now, so we can execute
          var module = load.execute.apply(null, depModules);
          if (!(module instanceof Module))
            throw new TypeError('Execution must define a Module instance');
          load.module = module;
          load.status = 'linked';
          finishLoad(loader, load);
        }
      }

This approach combined with the fix in #85 is working for the es6-module-loader now. Further information on corrections greatly appreciated, this is simply an interim measure pending further info.

[jorendorff]

At some point @dherman and I decided we would call execute with no arguments, contra older documentation.

I don't remember the details of the discussion. I think there was a question of what to do in the case of circular dependencies, and the easiest way to resolve that was just not to pass the arguments.

Of course any given execute hook can easily look up the dependencies if it needs them.

[guybedford]

The only issue here is that deps is an unnormalized array of dependencies, so this would require the user to manually normalize dependencies causing normalization to run twice. If going this way, perhaps provide the normalized dependency names as arguments.

Is there a way to handle circular dependencies through custom execution in this way? I can't actually see a way for the bindings to work with an eval system at all, so had assumed that circular dependencies were just off the cards for dynamic modules.

[guybedford]

Note, just in case it is hidden in this issue, moving dynamic evaluation to EnsureEvaluated should really be done as well.

[jorendorff]

If going this way, perhaps provide the normalized dependency names as arguments.

That sounds nice to me! What do you think, @dherman?

I can't actually see a way for the bindings to work with an eval system at all, so had assumed that circular dependencies were just off the cards for dynamic modules.

I don't think it's fully possible, but using proxies you can get pretty close. We decided to let userspace AMD-compatibility libraries decide how much magic they want to use.

[dherman]

I agree, sounds great!

[jorendorff]

Assigning to myself. I will refile the bug on bugs.ecmascript.org.

[guybedford]

My current proposal for this is the following:

Let instantiate take the form

{
  deps: [...unnormalized dependencies..],
  exports: ['these', 'are', 'the', 'export', 'names'],
  execute: function(exports, normalizedDeps) {

    // provide exports that circular dependents can see
    exports.value = 'here';

    // this call makes execution happen in turn, supporting circular refs
    System.get(normalizedDeps[0]); 

    // no need to return anything - exports is by reference
  }
}

By providing the exports up front, the dynamic module can be created during linking, as a Module object with getters to a plain JavaScript object. This module is then executed during EnsureEvaluated, allowing for deferred execution to work out.

Additionally, by executing these modules in EnsureEvaluated, System.get can be used as the execution driver in the execute function of the instantiate hook, supporting circular references elegantly.

The arguments to the execute function are the plain JavaScript object representing the module, and the list of normalized dependency names.

Would be great to hear feedback on this concept.

[unscriptable]

Ok, so now we're not forcing dynamic modules to provide their own registry? Can you explain exactly when (which hook) an AMD factory function should execute -- or a CommonJS module be evaluated? Obviously, it has to be before instantiate or we can't know the export names. Thanks for helping me understand. :)

[guybedford]

@unscriptable sorry to confuse, this is just my personal proposal. The current spec is for a side registry. My proposal is to provide the export names upfront and that avoids the need for a side registry allowing execution when calling System.get. The proposal is for evaluation to happen in execute after instantiate. The idea being that AMD and CommonJS export onto the default module name anyway (due to the inability to detect the difference between named exports, and a custom exports object in AMD and CommonJS).

[unscriptable]

At first I hated the idea of a side registry. It feels like a hole in the spec. However, the side registry solves some of issues we were having. I'm very happy with the side registry concept, atm.

The idea being that AMD and CommonJS export onto the default module name anyway (due to the inability to detect the difference between named exports, and a custom exports object in AMD and CommonJS).

I don't think this is the case. It's fairly easy to detect when a CommonJS module (or the CommonJS-like variant of AMD) exports using the exports object rather than module.exports. The former is a clear indication that the author intended named exported values, while the latter makes it obvious that the author intended a default exported value.

[guybedford]

The thing is that we still need to be accessing .default for the default export case.

Personally I've found it quite useful to have a flag to indicate that we always just want to deal in the module default property:

  Module({
    default: CommonJSModule,
    __useDefault: true
  });

I then override the System.import method, to return the default property of a module when the __useDefault flag is set, and a similar logic in instantiate.

[guybedford]

Moving the discussion to https://bugs.ecmascript.org/show_bug.cgi?id=2648