diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 47d4fbf677e5eb..a0a39de45b2a59 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1656,9 +1656,18 @@ E('ERR_QUIC_ENDPOINT_CLOSED', 'QUIC endpoint closed: %s (%d)', Error); E('ERR_QUIC_OPEN_STREAM_FAILED', 'Failed to open QUIC stream', Error); E('ERR_QUIC_TRANSPORT_ERROR', 'A QUIC transport error occurred. %d [%s]', Error); E('ERR_QUIC_VERSION_NEGOTIATION_ERROR', 'The QUIC session requires version negotiation', Error); -E('ERR_REQUIRE_ASYNC_MODULE', 'require() cannot be used on an ESM ' + +E('ERR_REQUIRE_ASYNC_MODULE', function(filename, parentFilename) { + let message = 'require() cannot be used on an ESM ' + 'graph with top-level await. Use import() instead. To see where the' + - ' top-level await comes from, use --experimental-print-required-tla.', Error); + ' top-level await comes from, use --experimental-print-required-tla.'; + if (parentFilename) { + message += `\n From ${parentFilename} `; + } + if (filename) { + message += `\n Requiring ${filename} `; + } + return message; +}, Error); E('ERR_REQUIRE_CYCLE_MODULE', '%s', Error); E('ERR_REQUIRE_ESM', function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) { diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index a3b437ade87c75..3823f73597aba2 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -338,20 +338,37 @@ class ModuleLoader { // TODO(joyeecheung): ensure that imported synchronous graphs are evaluated // synchronously so that any previously imported synchronous graph is already // evaluated at this point. + // TODO(joyeecheung): add something similar to CJS loader's requireStack to help + // debugging the the problematic links in the graph for import. if (job !== undefined) { mod[kRequiredModuleSymbol] = job.module; + const parentFilename = urlToFilename(parent?.filename); + // TODO(node:55782): this race may stop to happen when the ESM resolution and loading become synchronous. + if (!job.module) { + let message = `Cannot require() ES Module ${filename} because it is not yet fully loaded. `; + message += 'This may be caused by a race condition if the module is simultaneously dynamically '; + message += 'import()-ed via Promise.all(). Try await-ing the import() sequentially in a loop instead.'; + if (parentFilename) { + message += ` (from ${parentFilename})`; + } + assert(job.module, message); + } if (job.module.async) { - throw new ERR_REQUIRE_ASYNC_MODULE(); + throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename); } + // job.module may be undefined if it's asynchronously loaded. Which means + // there is likely a cycle. if (job.module.getStatus() !== kEvaluated) { - const parentFilename = urlToFilename(parent?.filename); let message = `Cannot require() ES Module ${filename} in a cycle.`; if (parentFilename) { message += ` (from ${parentFilename})`; } + message += 'A cycle involving require(esm) is disallowed to maintain '; + message += 'invariants madated by the ECMAScript specification'; + message += 'Try making at least part of the dependency in the graph lazily loaded.'; throw new ERR_REQUIRE_CYCLE_MODULE(message); } - return { wrap: job.module, namespace: job.module.getNamespaceSync() }; + return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) }; } // TODO(joyeecheung): refactor this so that we pre-parse in C++ and hit the // cache here, or use a carrier object to carry the compiled module script @@ -363,7 +380,7 @@ class ModuleLoader { job = new ModuleJobSync(this, url, kEmptyObject, wrap, isMain, inspectBrk); this.loadCache.set(url, kImplicitTypeAttribute, job); mod[kRequiredModuleSymbol] = job.module; - return { wrap: job.module, namespace: job.runSync().namespace }; + return { wrap: job.module, namespace: job.runSync(parent).namespace }; } /** diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 846a336d27547e..9a8800c78407a4 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -36,6 +36,7 @@ const assert = require('internal/assert'); const resolvedPromise = PromiseResolve(); const { setHasStartedUserESMExecution, + urlToFilename, } = require('internal/modules/helpers'); const { getOptionValue } = require('internal/options'); const noop = FunctionPrototype; @@ -380,7 +381,7 @@ class ModuleJobSync extends ModuleJobBase { `Status = ${status}`); } - runSync() { + runSync(parent) { // TODO(joyeecheung): add the error decoration logic from the async instantiate. this.module.async = this.module.instantiateSync(); // If --experimental-print-required-tla is true, proceeds to evaluation even @@ -389,11 +390,13 @@ class ModuleJobSync extends ModuleJobBase { // TODO(joyeecheung): track the asynchroniticy using v8::Module::HasTopLevelAwait() // and we'll be able to throw right after compilation of the modules, using acron // to find and print the TLA. + const parentFilename = urlToFilename(parent?.filename); + const filename = urlToFilename(this.url); if (this.module.async && !getOptionValue('--experimental-print-required-tla')) { - throw new ERR_REQUIRE_ASYNC_MODULE(); + throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename); } setHasStartedUserESMExecution(); - const namespace = this.module.evaluateSync(); + const namespace = this.module.evaluateSync(filename, parentFilename); return { __proto__: null, module: this.module, namespace }; } } diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 649ec428e2dd6f..8ba5dc01228fef 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -710,7 +710,7 @@ void ModuleWrap::EvaluateSync(const FunctionCallbackInfo& args) { FPrintF(stderr, "%s\n", reason); } } - THROW_ERR_REQUIRE_ASYNC_MODULE(env); + THROW_ERR_REQUIRE_ASYNC_MODULE(env, args[0], args[1]); return; } @@ -740,7 +740,7 @@ void ModuleWrap::GetNamespaceSync(const FunctionCallbackInfo& args) { } if (module->IsGraphAsync()) { - return THROW_ERR_REQUIRE_ASYNC_MODULE(realm->env()); + return THROW_ERR_REQUIRE_ASYNC_MODULE(realm->env(), args[0], args[1]); } Local result = module->GetModuleNamespace(); args.GetReturnValue().Set(result); diff --git a/src/node_errors.h b/src/node_errors.h index 4d2667fb4dfdce..7fa0dbe3c707ad 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -215,10 +215,6 @@ ERRORS_WITH_CODE(V) "creating Workers") \ V(ERR_NON_CONTEXT_AWARE_DISABLED, \ "Loading non context-aware native addons has been disabled") \ - V(ERR_REQUIRE_ASYNC_MODULE, \ - "require() cannot be used on an ESM graph with top-level await. Use " \ - "import() instead. To see where the top-level await comes from, use " \ - "--experimental-print-required-tla.") \ V(ERR_SCRIPT_EXECUTION_INTERRUPTED, \ "Script execution was interrupted by `SIGINT`") \ V(ERR_TLS_PSK_SET_IDENTITY_HINT_FAILED, "Failed to set PSK identity hint") \ @@ -248,6 +244,28 @@ inline void THROW_ERR_SCRIPT_EXECUTION_TIMEOUT(Environment* env, THROW_ERR_SCRIPT_EXECUTION_TIMEOUT(env, message.str().c_str()); } +inline void THROW_ERR_REQUIRE_ASYNC_MODULE( + Environment* env, + v8::Local filename, + v8::Local parent_filename) { + static constexpr const char* prefix = + "require() cannot be used on an ESM graph with top-level await. Use " + "import() instead. To see where the top-level await comes from, use " + "--experimental-print-required-tla."; + std::string message = prefix; + if (!parent_filename.IsEmpty() && parent_filename->IsString()) { + Utf8Value utf8(env->isolate(), parent_filename); + message += "\n From "; + message += utf8.out(); + } + if (!filename.IsEmpty() && filename->IsString()) { + Utf8Value utf8(env->isolate(), filename); + message += "\n Requiring "; + message += +utf8.out(); + } + THROW_ERR_REQUIRE_ASYNC_MODULE(env, message.c_str()); +} + inline v8::Local ERR_BUFFER_TOO_LARGE(v8::Isolate* isolate) { char message[128]; snprintf(message, diff --git a/test/common/index.js b/test/common/index.js index cf920fcc49d258..b8f5416c625e4c 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -855,6 +855,17 @@ function expectRequiredModule(mod, expectation, checkESModule = true) { assert.deepStrictEqual(clone, { ...expectation }); } +function expectRequiredTLAError(err) { + const message = /require\(\) cannot be used on an ESM graph with top-level await/; + if (typeof err === 'string') { + assert.match(err, /ERR_REQUIRE_ASYNC_MODULE/); + assert.match(err, message); + } else { + assert.strictEqual(err.code, 'ERR_REQUIRE_ASYNC_MODULE'); + assert.match(err.message, message); + } +} + const common = { allowGlobals, buildType, @@ -864,6 +875,7 @@ const common = { escapePOSIXShell, expectsError, expectRequiredModule, + expectRequiredTLAError, expectWarning, getArrayBufferViews, getBufferSources, diff --git a/test/es-module/test-require-module-tla-execution.js b/test/es-module/test-require-module-tla-execution.js new file mode 100644 index 00000000000000..bdd222d1f8fa0b --- /dev/null +++ b/test/es-module/test-require-module-tla-execution.js @@ -0,0 +1,26 @@ +'use strict'; + +// Tests that require(esm) with top-level-await throws before execution starts +// if --experimental-print-required-tla is not enabled. + +const common = require('../common'); +const assert = require('assert'); +const { spawnSyncAndExit } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +{ + spawnSyncAndExit(process.execPath, [ + fixtures.path('es-modules/tla/require-execution.js'), + ], { + signal: null, + status: 1, + stderr(output) { + assert.doesNotMatch(output, /I am executed/); + common.expectRequiredTLAError(output); + assert.match(output, /From .*require-execution\.js/); + assert.match(output, /Requiring .*execution\.mjs/); + return true; + }, + stdout: '' + }); +} diff --git a/test/es-module/test-require-module-tla-nested.js b/test/es-module/test-require-module-tla-nested.js new file mode 100644 index 00000000000000..583cd7cd0c95db --- /dev/null +++ b/test/es-module/test-require-module-tla-nested.js @@ -0,0 +1,15 @@ +'use strict'; + +// Tests that require(esm) throws for top-level-await in inner graphs. + +const common = require('../common'); +const assert = require('assert'); + +assert.throws(() => { + require('../fixtures/es-modules/tla/parent.mjs'); +}, (err) => { + common.expectRequiredTLAError(err); + assert.match(err.message, /From .*test-require-module-tla-nested\.js/); + assert.match(err.message, /Requiring .*parent\.mjs/); + return true; +}); diff --git a/test/es-module/test-require-module-tla-print-execution.js b/test/es-module/test-require-module-tla-print-execution.js new file mode 100644 index 00000000000000..40992ae32e0905 --- /dev/null +++ b/test/es-module/test-require-module-tla-print-execution.js @@ -0,0 +1,29 @@ +'use strict'; + +// Tests that require(esm) with top-level-await throws after execution +// if --experimental-print-required-tla is enabled. + +const common = require('../common'); +const assert = require('assert'); +const { spawnSyncAndExit } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +{ + spawnSyncAndExit(process.execPath, [ + '--experimental-print-required-tla', + fixtures.path('es-modules/tla/require-execution.js'), + ], { + signal: null, + status: 1, + stderr(output) { + assert.match(output, /I am executed/); + common.expectRequiredTLAError(output); + assert.match(output, /Error: unexpected top-level await at.*execution\.mjs:3/); + assert.match(output, /await Promise\.resolve\('hi'\)/); + assert.match(output, /From .*require-execution\.js/); + assert.match(output, /Requiring .*execution\.mjs/); + return true; + }, + stdout: '' + }); +} diff --git a/test/es-module/test-require-module-tla-rejected.js b/test/es-module/test-require-module-tla-rejected.js new file mode 100644 index 00000000000000..0c1f8de2c307f6 --- /dev/null +++ b/test/es-module/test-require-module-tla-rejected.js @@ -0,0 +1,15 @@ +'use strict'; + +// Tests that require(esm) throws for rejected top-level await. + +const common = require('../common'); +const assert = require('assert'); + +assert.throws(() => { + require('../fixtures/es-modules/tla/rejected.mjs'); +}, (err) => { + common.expectRequiredTLAError(err); + assert.match(err.message, /From .*test-require-module-tla-rejected\.js/); + assert.match(err.message, /Requiring .*rejected\.mjs/); + return true; +}); diff --git a/test/es-module/test-require-module-tla-resolved.js b/test/es-module/test-require-module-tla-resolved.js new file mode 100644 index 00000000000000..f35bb68b7dc180 --- /dev/null +++ b/test/es-module/test-require-module-tla-resolved.js @@ -0,0 +1,15 @@ +'use strict'; + +// Tests that require(esm) throws for resolved top-level-await. + +const common = require('../common'); +const assert = require('assert'); + +assert.throws(() => { + require('../fixtures/es-modules/tla/resolved.mjs'); +}, (err) => { + common.expectRequiredTLAError(err); + assert.match(err.message, /From .*test-require-module-tla-resolved\.js/); + assert.match(err.message, /Requiring .*resolved\.mjs/); + return true; +}); diff --git a/test/es-module/test-require-module-tla-unresolved.js b/test/es-module/test-require-module-tla-unresolved.js new file mode 100644 index 00000000000000..35cf12c446129b --- /dev/null +++ b/test/es-module/test-require-module-tla-unresolved.js @@ -0,0 +1,15 @@ +'use strict'; + +// Tests that require(esm) throws for unresolved top-level-await. + +const common = require('../common'); +const assert = require('assert'); + +assert.throws(() => { + require('../fixtures/es-modules/tla/unresolved.mjs'); +}, (err) => { + common.expectRequiredTLAError(err); + assert.match(err.message, /From .*test-require-module-tla-unresolved\.js/); + assert.match(err.message, /Requiring .*unresolved\.mjs/); + return true; +}); diff --git a/test/es-module/test-require-module-tla.js b/test/es-module/test-require-module-tla.js deleted file mode 100644 index 9b38b1cab3fcb5..00000000000000 --- a/test/es-module/test-require-module-tla.js +++ /dev/null @@ -1,63 +0,0 @@ -// Flags: --experimental-require-module -'use strict'; - -require('../common'); -const assert = require('assert'); -const { spawnSyncAndExit } = require('../common/child_process'); -const fixtures = require('../common/fixtures'); - -const message = /require\(\) cannot be used on an ESM graph with top-level await/; -const code = 'ERR_REQUIRE_ASYNC_MODULE'; - -assert.throws(() => { - require('../fixtures/es-modules/tla/rejected.mjs'); -}, { message, code }); - -assert.throws(() => { - require('../fixtures/es-modules/tla/unresolved.mjs'); -}, { message, code }); - - -assert.throws(() => { - require('../fixtures/es-modules/tla/resolved.mjs'); -}, { message, code }); - -// Test TLA in inner graphs. -assert.throws(() => { - require('../fixtures/es-modules/tla/parent.mjs'); -}, { message, code }); - -{ - spawnSyncAndExit(process.execPath, [ - '--experimental-require-module', - fixtures.path('es-modules/tla/require-execution.js'), - ], { - signal: null, - status: 1, - stderr(output) { - assert.doesNotMatch(output, /I am executed/); - assert.match(output, message); - return true; - }, - stdout: '' - }); -} - -{ - spawnSyncAndExit(process.execPath, [ - '--experimental-require-module', - '--experimental-print-required-tla', - fixtures.path('es-modules/tla/require-execution.js'), - ], { - signal: null, - status: 1, - stderr(output) { - assert.match(output, /I am executed/); - assert.match(output, /Error: unexpected top-level await at.*execution\.mjs:3/); - assert.match(output, /await Promise\.resolve\('hi'\)/); - assert.match(output, message); - return true; - }, - stdout: '' - }); -}