revert back to former registerDynamic form to ensure a smooth upgrade path

Author: guybedford

core/register-loader.js

@@ -67,6 +67,8 @@ function createLoadRecord (state, key, registration) {
       instantiatePromise: undefined,
       dependencies: undefined,
       execute: undefined,
+      executingRequire: false,
+
       // underlying module object bindings
       moduleObj: undefined,
 
@@ -210,7 +212,8 @@ function instantiate (loader, load, link, registry, state) {
     if (registration[2]) {
       link.moduleObj.default = {};
       link.moduleObj.__useDefault = true;
-      link.execute = registration[1];
+      link.executingRequire = registration[1];
+      link.execute = registration[2];
     }
 
     // process System.register declaration
@@ -452,48 +455,34 @@ RegisterLoader.prototype.register = function (key, deps, declare) {
 
   // anonymous modules get stored as lastAnon
   if (declare === undefined) {
-    state.lastRegister = [key, deps, false];
+    state.lastRegister = [key, deps, undefined];
   }
 
   // everything else registers into the register cache
   else {
     var load = state.records[key] || createLoadRecord(state, key, undefined);
-    load.registration = [deps, declare, false];
+    load.registration = [deps, declare, undefined];
   }
 };
 
 /*
  * System.registerDyanmic
  */
-RegisterLoader.prototype.registerDynamic = function (key, deps, execute) {
+RegisterLoader.prototype.registerDynamic = function (key, deps, executingRequire, execute) {
   var state = this[REGISTER_INTERNAL];
 
   // anonymous modules get stored as lastAnon
   if (typeof key !== 'string') {
-    state.lastRegister = [key, typeof deps === 'boolean' ? dynamicExecuteCompat(key, deps, execute) : deps, true];
+    state.lastRegister = [key, deps, executingRequire];
   }
 
   // everything else registers into the register cache
   else {
     var load = state.records[key] || createLoadRecord(state, key, undefined);
-    load.registration = [deps, typeof execute === 'boolean' ? dynamicExecuteCompat(deps, execute, arguments[3]) : execute, true];
+    load.registration = [deps, executingRequire, execute];
   }
 };
 
-function dynamicExecuteCompat (deps, executingRequire, execute) {
-  return function (require, exports, module) {
-    // evaluate deps first
-    if (!executingRequire)
-      for (var i = 0; i < deps.length; i++)
-        require(deps[i]);
-
-    // then run execution function
-    // also provide backwards compat for no return value
-    // previous 4 argument form of System.register had "this" as global value
-    module.exports = execute.apply(global, arguments) || module.exports;
-  };
-}
-
 // ContextualLoader class
 // backwards-compatible with previous System.register context argument by exposing .id
 function ContextualLoader (loader, key) {
@@ -526,7 +515,7 @@ function ensureEvaluate (loader, load, link, registry, state, seen) {
 
   // for ES loads we always run ensureEvaluate on top-level, so empty seen is passed regardless
   // for dynamic loads, we pass seen if also dynamic
-  var err = doEvaluate(loader, load, link, registry, state, load.setters ? [] : seen || []);
+  var err = doEvaluate(loader, load, link, registry, state, link.setters ? [] : seen || []);
   if (err) {
     clearLoadErrors(loader, load);
     throw err;
@@ -593,7 +582,7 @@ function doEvaluate (loader, load, link, registry, state, seen) {
     // ES System.register execute
     // "this" is null in ES
     if (link.setters) {
-      err = doExecute(link.execute, nullContext);
+      err = declarativeExecute(link.execute);
     }
     // System.registerDynamic execute
     // "this" is "exports" in CJS
@@ -609,11 +598,15 @@ function doEvaluate (loader, load, link, registry, state, seen) {
           return moduleObj.default;
         }
       });
-      err = doExecute(link.execute, module.exports, [
-        makeDynamicRequire(loader, load.key, link.dependencies, link.dependencyInstantiations, registry, state, seen),
-        module.exports,
-        module
-      ]);
+
+      var require = makeDynamicRequire(loader, load.key, link.dependencies, link.dependencyInstantiations, registry, state, seen);
+
+      // evaluate deps first
+      if (!link.executingRequire)
+        for (var i = 0; i < link.dependencies.length; i++)
+          require(link.dependencies[i]);
+
+      err = dynamicExecute(link.execute, require, moduleObj.default, module);
       // __esModule flag extension support
       if (moduleObj.default && moduleObj.default.__esModule)
         for (var p in moduleObj.default)
@@ -645,9 +638,21 @@ function doEvaluate (loader, load, link, registry, state, seen) {
 var nullContext = {};
 if (Object.freeze)
   Object.freeze(nullContext);
-function doExecute (execute, context, args) {
+
+function declarativeExecute (execute) {
+  try {
+    execute.call(nullContext);
+  }
+  catch (e) {
+    return e;
+  }
+}
+
+function dynamicExecute (execute, require, exports, module) {
   try {
-    execute.apply(context, args);
+    var output = execute.call(global, require, exports, module);
+    if (output !== undefined)
+      module.exports = output;
   }
   catch (e) {
     return e;

docs/system-register-dynamic.md

@@ -17,7 +17,7 @@ optional. This allows it to work as both a bundle transport format and a wrapper
 The format of `System.registerDynamic` is designed to closely match CommonJS:
 
 ```javascript
-System.registerDynamic('optional-name', ['unnormalized-dependency'], function (require, exports, module) {
+System.registerDynamic('optional-name', ['unnormalized-dependency'], true, function (require, exports, module) {
   // require is executing - the dependency is only executed when we hit this require
   // Note that we can only require modules that have already been declared through the dependencies
   // array above. This is because synchronous module instantiation is not supported in the loader.
@@ -37,10 +37,4 @@ System.registerDynamic('optional-name', ['unnormalized-dependency'], function (r
 ```
 
 The resultant `ModuleNamespace` object is taken to be the object with the `default` export equal to the `module.exports` value
-from the CommonJS module, and this object is created once execution has completed only.
-
-The `default` binding is still made available to ES modules that might depend on this CommonJS module while execution is in progress,
-so that `import x from 'cjs'` will have a defined binding to `module.exports` even while `cjs` is still executing.
-
-Once executed, the named exports are populated from the iterable properties of `module.exports` so that `import {readFile} from 'fs'`
-can work as long as `fs` is fully executed at the time of `readFile` being accessed.
+from the CommonJS module, with this property set to the exports object from the beginning of the linking phase.