From 41e9ee857ebf7c3e693126e504d0a3faab39da80 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 21 Feb 2017 10:54:04 -0500 Subject: [PATCH] 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 | 16 ++++++- tools/isobuild/linker.js | 44 +++++++++++++++++-- .../apps/dynamic-import/.meteor/packages | 1 + .../packages/helper-package/dynamic/a.js | 3 ++ .../packages/helper-package/dynamic/b.coffee | 3 ++ .../packages/helper-package/helper-package.js | 15 +++++++ .../packages/helper-package/package.js | 12 +++++ .../packages/lazy-test-package/dynamic.js | 7 +++ .../packages/lazy-test-package/package.js | 2 + tools/tests/apps/dynamic-import/tests.js | 22 ++++++++++ 10 files changed, 120 insertions(+), 5 deletions(-) create mode 100644 tools/tests/apps/dynamic-import/packages/helper-package/dynamic/a.js create mode 100644 tools/tests/apps/dynamic-import/packages/helper-package/dynamic/b.coffee create mode 100644 tools/tests/apps/dynamic-import/packages/helper-package/helper-package.js create mode 100644 tools/tests/apps/dynamic-import/packages/helper-package/package.js diff --git a/packages/dynamic-import/client.js b/packages/dynamic-import/client.js index 2d96067a2d..325e16dae0 100644 --- a/packages/dynamic-import/client.js +++ b/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. diff --git a/tools/isobuild/linker.js b/tools/isobuild/linker.js index 84597aee8a..c00c1eaa66 100644 --- a/tools/isobuild/linker.js +++ b/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("]; diff --git a/tools/tests/apps/dynamic-import/.meteor/packages b/tools/tests/apps/dynamic-import/.meteor/packages index 6732427412..bf003114fb 100644 --- a/tools/tests/apps/dynamic-import/.meteor/packages +++ b/tools/tests/apps/dynamic-import/.meteor/packages @@ -24,3 +24,4 @@ dynamic-import dispatch:mocha-phantomjs dispatch:mocha-browser lazy-test-package +helper-package diff --git a/tools/tests/apps/dynamic-import/packages/helper-package/dynamic/a.js b/tools/tests/apps/dynamic-import/packages/helper-package/dynamic/a.js new file mode 100644 index 0000000000..50d7cacfac --- /dev/null +++ b/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 } diff --git a/tools/tests/apps/dynamic-import/packages/helper-package/dynamic/b.coffee b/tools/tests/apps/dynamic-import/packages/helper-package/dynamic/b.coffee new file mode 100644 index 0000000000..504ae3344a --- /dev/null +++ b/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 diff --git a/tools/tests/apps/dynamic-import/packages/helper-package/helper-package.js b/tools/tests/apps/dynamic-import/packages/helper-package/helper-package.js new file mode 100644 index 0000000000..9f689d2b74 --- /dev/null +++ b/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"; + } +}; diff --git a/tools/tests/apps/dynamic-import/packages/helper-package/package.js b/tools/tests/apps/dynamic-import/packages/helper-package/package.js new file mode 100644 index 0000000000..28e93af859 --- /dev/null +++ b/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"); +}); diff --git a/tools/tests/apps/dynamic-import/packages/lazy-test-package/dynamic.js b/tools/tests/apps/dynamic-import/packages/lazy-test-package/dynamic.js index f3e9aa9c6f..038673028a 100644 --- a/tools/tests/apps/dynamic-import/packages/lazy-test-package/dynamic.js +++ b/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"); +} diff --git a/tools/tests/apps/dynamic-import/packages/lazy-test-package/package.js b/tools/tests/apps/dynamic-import/packages/lazy-test-package/package.js index 3f740f2ec8..ec3a50664b 100644 --- a/tools/tests/apps/dynamic-import/packages/lazy-test-package/package.js +++ b/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" diff --git a/tools/tests/apps/dynamic-import/tests.js b/tools/tests/apps/dynamic-import/tests.js index e04ef67f1a..0905ce7386 100644 --- a/tools/tests/apps/dynamic-import/tests.js +++ b/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 + ); + }); });