Evaluate dynamic module code in package scope.

This should elegantly address the issues described in this comment:
https://github.com/meteor/meteor/pull/8327#issuecomment-280881830

I toyed with the possibility of turning package variables (both imports
from other packages and intercepted "global" variable assignments) into
properties on a shared namespace object, but that would have been a major
breaking change for existing package code, because it would have required
automatically rewriting variable references in package modules.

packages/dynamic-import/client.js

@@ -90,7 +90,21 @@ function installResults(resultsTree) {
       addToTree(
         trees[optionsIndex],
         meta.module.id,
-        (0, eval)("(" + tree + ")")
+        // By calling (meta.options.eval || eval) in a wrapper function,
+        // we delay the cost of parsing and evaluating the module code
+        // until the module is first imported.
+        function () {
+          // If an options.eval function was provided in the second
+          // argument to meteorInstall when this bundle was first
+          // installed, use that function to parse and evaluate the
+          // dynamic module code in the scope of the package. Otherwise
+          // fall back to indirect (global) eval.
+          return (meta.options.eval || eval)(
+            // Wrap the function(require,exports,module){...} expression
+            // in parentheses to force it to be parsed as an expression.
+            "(" + tree + ")"
+          ).apply(this, arguments);
+        }
       );
 
       // Intentionally do not delay resolution waiting for the cache.

tools/isobuild/linker.js

@@ -245,9 +245,7 @@ _.extend(Module.prototype, {
         return;
       }
 
-      const dynamic = file.lazy && file.imported === "dynamic";
-
-      if (dynamic) {
+      if (file.isDynamic()) {
         const servePath = "dynamic/" + file.installPath;
         const { code: source, map } =
           file.getPrelinkedOutput({
@@ -352,7 +350,7 @@ _.extend(Module.prototype, {
     // allows us to call meteorInstall just once to install everything.
     chunks.push("var require = meteorInstall(");
     walk(tree);
-    chunks.push(",", JSON.stringify(self.meteorInstallOptions), ");");
+    chunks.push(",", self._stringifyInstallOptions(), ");");
 
     if (moduleCount === 0) {
       // If no files were actually added to the chunks array, roll back
@@ -363,6 +361,40 @@ _.extend(Module.prototype, {
     return moduleCount;
   },
 
+  _stringifyInstallOptions() {
+    let optionsString =
+      JSON.stringify(this.meteorInstallOptions, null, 2);
+
+    if (this.useGlobalNamespace) {
+      return optionsString;
+    }
+
+    if (! this.files.some(file => file.isDynamic())) {
+      // If the package contains no files that can be imported
+      // dynamically, then we don't need to provide an options.eval
+      // function for evaluating dynamic modules.
+      return optionsString;
+    }
+
+    assert.ok(optionsString.endsWith("\n}"));
+
+    // If this package is not using the global namespace, pass an
+    // options.eval method to meteorInstall, so that code added later can
+    // have access to the same shared package variables as other code in
+    // the package.
+    return optionsString.slice(0, optionsString.length - 2) + [
+      ",",
+      "  eval: function () {",
+      "    return eval(arguments[0]);",
+      "  }",
+      "}"
+    ].join("\n");
+  },
+
+  _hasDynamicModules() {
+    return this.files.some(file => file.isDynamic());
+  },
+
   // Adds require calls to the chunks array for all modules that should be
   // eagerly evaluated, and also includes bare files in the appropriate
   // order with respect to the require calls. Returns the name of the
@@ -583,6 +615,10 @@ _.extend(File.prototype, {
     return this.module.meteorInstallOptions;
   },
 
+  isDynamic() {
+    return this.lazy && this.imported === "dynamic";
+  },
+
   _getClosureHeader() {
     if (this._useMeteorInstall()) {
       const headerParts = ["function("];

tools/tests/apps/dynamic-import/.meteor/packages

@@ -24,3 +24,4 @@ dynamic-import
 dispatch:mocha-phantomjs
 dispatch:mocha-browser
 lazy-test-package
+helper-package

tools/tests/apps/dynamic-import/packages/helper-package/dynamic/a.js

@@ -0,0 +1,3 @@
+sharedWithinHelperPackage = sharedWithinHelperPackage || {};
+sharedWithinHelperPackage[module.id] = true;
+export { sharedWithinHelperPackage as shared }

tools/tests/apps/dynamic-import/packages/helper-package/dynamic/b.coffee

@@ -0,0 +1,3 @@
+localShared = require("./a.js").shared
+localShared[module.id] = true
+exports.shared = localShared

tools/tests/apps/dynamic-import/packages/helper-package/helper-package.js

@@ -0,0 +1,15 @@
+import assert from "assert";
+
+// Maks sure sharedWithinHelperPackage and __coffeescriptShare are
+// declared as variables in the private scope of this package, but not
+// defined globally.
+assert.strictEqual(sharedWithinHelperPackage, void 0);
+assert.strictEqual("sharedWithinHelperPackage" in global, false);
+assert.strictEqual(__coffeescriptShare, void 0);
+assert.strictEqual("__coffeescriptShare" in global, false);
+
+export const Helper = {
+  help() {
+    return "ok";
+  }
+};

tools/tests/apps/dynamic-import/packages/helper-package/package.js

@@ -0,0 +1,12 @@
+Package.describe({
+  name: 'helper-package',
+  version: '0.0.1',
+});
+
+Package.onUse(function(api) {
+//  api.versionsFrom('1.4.2.7');
+  api.use('ecmascript');
+  api.use('coffeescript');
+  api.mainModule('helper-package.js');
+  api.export("Helper");
+});

tools/tests/apps/dynamic-import/packages/lazy-test-package/dynamic.js

@@ -1 +1,8 @@
+import assert from "assert";
+
 export const name = module.id;
+
+export function checkHelper() {
+  assert.strictEqual(typeof Helper, "object");
+  assert.strictEqual(Helper.help(), "ok");
+}

tools/tests/apps/dynamic-import/packages/lazy-test-package/package.js

@@ -5,6 +5,8 @@ Package.describe({
 
 Package.onUse(function(api) {
   api.use("ecmascript");
+  api.use("helper-package");
+
   api.mainModule("main.js", [
     "client",
     "server"

tools/tests/apps/dynamic-import/tests.js

@@ -124,4 +124,26 @@ describe("dynamic import(...)", function () {
       })
     ]);
   });
+
+  it("gives dynamic modules access to package variables", async function () {
+    const dynamic = await import("meteor/lazy-test-package/dynamic");
+    dynamic.checkHelper();
+
+    const a = await import("meteor/helper-package/dynamic/a");
+    const b = await import("meteor/helper-package/dynamic/b.coffee");
+
+    assert.strictEqual(a.shared, b.shared);
+    assert.deepEqual(a.shared, {
+      "/node_modules/meteor/helper-package/dynamic/a.js": true,
+      "/node_modules/meteor/helper-package/dynamic/b.coffee.js": true
+    });
+
+    assert.strictEqual(
+      (await import("meteor/helper-package")).Helper,
+      // Since these tests are defined in an application that uses the
+      // global scope for imported package variables, global.Helper should
+      // be identical to the Helper symbol exported by helper-package.
+      global.Helper
+    );
+  });
 });