From 219964b08c41eca4f7bae75ec8864b19edee3590 Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Tue, 10 Dec 2019 13:45:16 -0800 Subject: [PATCH 1/5] module: support requiring .mjs files This implements the ability to use require on .mjs files, loaded via the esm loader, using the same tradeoffs that top level await makes in esm itself. What this means: If possible, all execution and evaluation is done synchronously, via immediately unwrapping the execution's component promises. This means that any and all existing code should have no observable change in behavior, as there exist no asynchronous modules as of yet. The catch is that once a module which requires asynchronous execution is used, it must yield to the event loop to perform that execution, which, in turn, can allow other code to execute before the continuation after the async action, which is observable to callers of the now asynchronous module. If this matters to your callers, this means making your module execution asynchronous could be considered a breaking change to your library, however in practice, it will not matter for most callers. Moreover, as the ecosystem exists today, there are zero asynchronously executing modules, and so until there are, there are no downsides to this approach at all, as no execution is changed from what one would expect today (excepting, ofc, that it's no longer an error to require("./foo.mjs"). Ref: https://github.com/nodejs/modules/issues/308 Ref: https://github.com/nodejs/modules/issues/299 Ref: https://github.com/nodejs/modules/issues/454 --- lib/internal/modules/cjs/loader.js | 65 +++++++++++++++++------------- lib/internal/modules/esm/loader.js | 8 +++- src/node_task_queue.cc | 41 +++++++++++++++++++ 3 files changed, 85 insertions(+), 29 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index f4112a6a0e6717..06c18a37c3b680 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -86,6 +86,7 @@ const { ERR_REQUIRE_ESM } = require('internal/errors').codes; const { validateString } = require('internal/validators'); +const { promiseWait } = internalBinding('task_queue'); const pendingDeprecation = getOptionValue('--pending-deprecation'); module.exports = { @@ -1038,36 +1039,34 @@ Module.prototype.load = function(filename) { this.paths = Module._nodeModulePaths(path.dirname(filename)); const extension = findLongestRegisteredExtension(filename); - // allow .mjs to be overridden - if (filename.endsWith('.mjs') && !Module._extensions['.mjs']) { - throw new ERR_REQUIRE_ESM(filename); - } Module._extensions[extension](this, filename); this.loaded = true; - const ESMLoader = asyncESM.ESMLoader; - const url = `${pathToFileURL(filename)}`; - const module = ESMLoader.moduleMap.get(url); - // Create module entry at load time to snapshot exports correctly - const exports = this.exports; - // Called from cjs translator - if (module !== undefined && module.module !== undefined) { - if (module.module.getStatus() >= kInstantiated) - module.module.setExport('default', exports); - } else { - // Preemptively cache - // We use a function to defer promise creation for async hooks. - ESMLoader.moduleMap.set( - url, - // Module job creation will start promises. - // We make it a function to lazily trigger those promises - // for async hooks compatibility. - () => new ModuleJob(ESMLoader, url, () => - new ModuleWrap(url, undefined, ['default'], function() { - this.setExport('default', exports); - }) - , false /* isMain */, false /* inspectBrk */) - ); + if (extension !== '.mjs') { + const ESMLoader = asyncESM.ESMLoader; + const url = `${pathToFileURL(filename)}`; + const module = ESMLoader.moduleMap.get(url); + // Create module entry at load time to snapshot exports correctly + const exports = this.exports; + // Called from cjs translator + if (module !== undefined && module.module !== undefined) { + if (module.module.getStatus() >= kInstantiated) + module.module.setExport('default', exports); + } else { + // Preemptively cache + // We use a function to defer promise creation for async hooks. + ESMLoader.moduleMap.set( + url, + // Module job creation will start promises. + // We make it a function to lazily trigger those promises + // for async hooks compatibility. + () => new ModuleJob(ESMLoader, url, () => + new ModuleWrap(url, undefined, ['default'], function() { + this.setExport('default', exports); + }) + , false /* isMain */, false /* inspectBrk */) + ); + } } }; @@ -1246,6 +1245,18 @@ Module._extensions['.node'] = function(module, filename) { return process.dlopen(module, path.toNamespacedPath(filename)); }; +Module._extensions['.mjs'] = function(module, filename) { + const ESMLoader = asyncESM.ESMLoader; + const url = `${pathToFileURL(filename)}`; + const job = ESMLoader.getModuleJobWorker( + url, + 'module' + ); + const instantiated = promiseWait(job.instantiate()); // so long as builtin esm resolve is sync, this will complete sync (if the loader is extensible, an async loader will be have an observable effect here) + module.exports = instantiated.getNamespace(); + promiseWait(instantiated.evaluate(-1, false)); // so long as the module doesn't contain TLA, this will be sync, otherwise it appears async +} + function createRequireFromPath(filename) { // Allow a directory to be passed as the filename const trailingSlash = diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 255e5d2aba7bd8..6068643eb6ce33 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -152,8 +152,7 @@ class Loader { } } - async getModuleJob(specifier, parentURL) { - const { url, format } = await this.resolve(specifier, parentURL); + getModuleJobWorker(url, format, parentURL) { let job = this.moduleMap.get(url); // CommonJS will set functions for lazy job evaluation. if (typeof job === 'function') @@ -188,6 +187,11 @@ class Loader { this.moduleMap.set(url, job); return job; } + + async getModuleJob(specifier, parentURL) { + const { url, format } = await this.resolve(specifier, parentURL); + return this.getModuleJobWorker(url, format, parentURL); + } } ObjectSetPrototypeOf(Loader.prototype, null); diff --git a/src/node_task_queue.cc b/src/node_task_queue.cc index f418a272470e2b..46613e04e4b029 100644 --- a/src/node_task_queue.cc +++ b/src/node_task_queue.cc @@ -119,6 +119,45 @@ static void SetPromiseRejectCallback( env->set_promise_reject_callback(args[0].As()); } +/** + * Immediately unwraps a promise into a return value or throw, if possible + * If not, runs the event loop and microtask queue until it is unwrapable. + */ +static void PromiseWait(const FunctionCallbackInfo& args) { + if (!args[0]->IsPromise()) { + args.GetReturnValue().Set(args[0]); + return; + } + v8::Local promise = args[0].As(); + if (promise->State() == v8::Promise::kFulfilled) { + args.GetReturnValue().Set(promise->Result()); + return; + } + Isolate* isolate = args.GetIsolate(); + if (promise->State() == v8::Promise::kRejected) { + isolate->ThrowException(promise->Result()); + return; + } + + Environment* env = Environment::GetCurrent(args); + + uv_loop_t* loop = env->event_loop(); + int state = promise->State(); + while (state == v8::Promise::kPending) { + isolate->RunMicrotasks(); + if (uv_loop_alive(loop) && promise->State() == v8::Promise::kPending) { + uv_run(loop, UV_RUN_ONCE); + } + state = promise->State(); + } + + if (promise->State() == v8::Promise::kRejected) { + isolate->ThrowException(promise->Result()); + return; + } + args.GetReturnValue().Set(promise->Result()); +} + static void Initialize(Local target, Local unused, Local context, @@ -145,6 +184,8 @@ static void Initialize(Local target, env->SetMethod(target, "setPromiseRejectCallback", SetPromiseRejectCallback); + + env->SetMethod(target, "promiseWait", PromiseWait); } } // namespace task_queue From 286692b89ea42b26ed459efd316ed1f08fc1fe0b Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Tue, 10 Dec 2019 14:23:02 -0800 Subject: [PATCH 2/5] Fix lints --- lib/internal/modules/cjs/loader.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 06c18a37c3b680..26a5b0c03fab9c 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1248,13 +1248,18 @@ Module._extensions['.node'] = function(module, filename) { Module._extensions['.mjs'] = function(module, filename) { const ESMLoader = asyncESM.ESMLoader; const url = `${pathToFileURL(filename)}`; - const job = ESMLoader.getModuleJobWorker( - url, - 'module' - ); - const instantiated = promiseWait(job.instantiate()); // so long as builtin esm resolve is sync, this will complete sync (if the loader is extensible, an async loader will be have an observable effect here) + const job = ESMLoader.getModuleJobWorker(url, 'module'); + /* So long as builtin esm resolve is sync, this will complete sync + * (if the loader is extensible, an async loader will be have an observable + * effect here) + */ + const instantiated = promiseWait(job.instantiate()); module.exports = instantiated.getNamespace(); - promiseWait(instantiated.evaluate(-1, false)); // so long as the module doesn't contain TLA, this will be sync, otherwise it appears async + /** + * So long as the module doesn't contain TLA, this will be sync, otherwise it + * appears async + */ + promiseWait(instantiated.evaluate(-1, false)); } function createRequireFromPath(filename) { From fe74e6f0b91f89ca3459c20d76da0e4ef43e7924 Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Tue, 10 Dec 2019 14:53:44 -0800 Subject: [PATCH 3/5] Remove unneeded call to `promiseWait` - we only need this once TLA support is in, and until then, the results are a bit off --- lib/internal/modules/cjs/loader.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 26a5b0c03fab9c..30bd3675dc6f84 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1255,11 +1255,7 @@ Module._extensions['.mjs'] = function(module, filename) { */ const instantiated = promiseWait(job.instantiate()); module.exports = instantiated.getNamespace(); - /** - * So long as the module doesn't contain TLA, this will be sync, otherwise it - * appears async - */ - promiseWait(instantiated.evaluate(-1, false)); + instantiated.evaluate(-1, false); } function createRequireFromPath(filename) { From afe50b98fddb1af985a6b36b3f01da756019bf92 Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Tue, 10 Dec 2019 14:56:09 -0800 Subject: [PATCH 4/5] Fix mjs test --- test/parallel/test-require-mjs.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/test/parallel/test-require-mjs.js b/test/parallel/test-require-mjs.js index 69f8527555db71..a6256faa6beaa6 100644 --- a/test/parallel/test-require-mjs.js +++ b/test/parallel/test-require-mjs.js @@ -2,10 +2,4 @@ require('../common'); const assert = require('assert'); -assert.throws( - () => require('../fixtures/es-modules/test-esm-ok.mjs'), - { - message: /Must use import to load ES Module/, - code: 'ERR_REQUIRE_ESM' - } -); +assert.equal(require('../fixtures/es-modules/test-esm-ok.mjs').default, true); From 0a1faa47ba6f0877ab7f9112fb272ea3118706e6 Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Tue, 10 Dec 2019 15:42:54 -0800 Subject: [PATCH 5/5] Fix lints, again --- lib/internal/modules/cjs/loader.js | 2 +- test/parallel/test-require-mjs.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 30bd3675dc6f84..38108ae34d16f9 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1256,7 +1256,7 @@ Module._extensions['.mjs'] = function(module, filename) { const instantiated = promiseWait(job.instantiate()); module.exports = instantiated.getNamespace(); instantiated.evaluate(-1, false); -} +}; function createRequireFromPath(filename) { // Allow a directory to be passed as the filename diff --git a/test/parallel/test-require-mjs.js b/test/parallel/test-require-mjs.js index a6256faa6beaa6..b855632810de67 100644 --- a/test/parallel/test-require-mjs.js +++ b/test/parallel/test-require-mjs.js @@ -2,4 +2,4 @@ require('../common'); const assert = require('assert'); -assert.equal(require('../fixtures/es-modules/test-esm-ok.mjs').default, true); +assert.strictEqual(require('../fixtures/es-modules/test-esm-ok.mjs').default, true);