From 3b2f28e43e9f4fbad05dce8bc713f68a736e352e Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Fri, 8 Mar 2024 21:53:50 -0800 Subject: [PATCH] module: fix detect-module not retrying as esm for cjs-only errors --- src/node_contextify.cc | 57 ++++++++++++++++++- test/es-module/test-esm-detect-ambiguous.mjs | 41 +++++++++++++ .../commonjs-wrapper-variables.js | 6 ++ 3 files changed, 101 insertions(+), 3 deletions(-) create mode 100644 test/fixtures/es-modules/package-without-type/commonjs-wrapper-variables.js diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 4585e759c0ec8de..c1b8b3d52c224aa 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -1398,6 +1398,25 @@ constexpr std::array esm_syntax_error_messages = { "Unexpected token 'export'", // `export` statements "Cannot use 'import.meta' outside a module"}; // `import.meta` references +// Another class of error messages that we need to check for are syntax errors +// where the syntax throws when parsed as CommonJS but succeeds when parsed as +// ESM. So far, the cases we've found are: +// - CommonJS module variables (`module`, `exports`, `require`, `__filename`, +// `__dirname`): if the user writes code such as `const module =` in the top +// level of a CommonJS module, it will throw a syntax error; but the same +// code is valid in ESM. +// - Top-level `await`: if the user writes `await` at the top level of a +// CommonJS module, it will throw a syntax error; but the same code is valid +// in ESM. +constexpr std::array throws_only_in_cjs_error_messages = { + "Identifier 'module' has already been declared", + "Identifier 'exports' has already been declared", + "Identifier 'require' has already been declared", + "Identifier '__filename' has already been declared", + "Identifier '__dirname' has already been declared", + "await is only valid in async functions and " + "the top level bodies of modules"}; + void ContextifyContext::ContainsModuleSyntax( const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -1470,19 +1489,51 @@ void ContextifyContext::ContainsModuleSyntax( id_symbol, try_catch); - bool found_error_message_caused_by_module_syntax = false; + bool should_retry_as_esm = false; if (try_catch.HasCaught() && !try_catch.HasTerminated()) { Utf8Value message_value(env->isolate(), try_catch.Message()->Get()); auto message = message_value.ToStringView(); for (const auto& error_message : esm_syntax_error_messages) { if (message.find(error_message) != std::string_view::npos) { - found_error_message_caused_by_module_syntax = true; + should_retry_as_esm = true; + break; + } + } + + for (const auto& error_message : throws_only_in_cjs_error_messages) { + if (message.find(error_message) != std::string_view::npos) { + // Try parsing again where the user's code is wrapped within an async + // function. If the new parse succeeds, then the error was caused by + // either a top-level declaration of one of the CommonJS module + // variables, or a top-level `await`. + TryCatchScope second_parse_try_catch(env); + Local wrapped_code = String::Concat(isolate, + String::NewFromUtf8(isolate, "(async function() {").ToLocalChecked(), + code); + wrapped_code = String::Concat(isolate, wrapped_code, + String::NewFromUtf8(isolate, "})();").ToLocalChecked()); + ScriptCompiler::Source wrapped_source = GetCommonJSSourceInstance( + isolate, wrapped_code, filename, 0, 0, host_defined_options, + nullptr); + ContextifyContext::CompileFunctionAndCacheResult(env, + context, + &wrapped_source, + std::move(params), + std::vector>(), + options, + true, + id_symbol, + second_parse_try_catch); + if (!second_parse_try_catch.HasCaught() && + !second_parse_try_catch.HasTerminated()) { + should_retry_as_esm = true; + } break; } } } - args.GetReturnValue().Set(found_error_message_caused_by_module_syntax); + args.GetReturnValue().Set(should_retry_as_esm); } static void StartSigintWatchdog(const FunctionCallbackInfo& args) { diff --git a/test/es-module/test-esm-detect-ambiguous.mjs b/test/es-module/test-esm-detect-ambiguous.mjs index 43537f26f5304a6..fef504df487bed0 100644 --- a/test/es-module/test-esm-detect-ambiguous.mjs +++ b/test/es-module/test-esm-detect-ambiguous.mjs @@ -234,6 +234,47 @@ describe('--experimental-detect-module', { concurrency: true }, () => { }); } }); + + // https://github.com/nodejs/node/issues/50917 + describe('syntax that errors in CommonJS but works in ESM', { concurrency: true }, () => { + it('permits top-level `await`', async () => { + const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [ + '--experimental-detect-module', + '--eval', + 'await Promise.resolve(); console.log("executed");', + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, 'executed\n'); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + it('still throws on `await` in an ordinary sync function', async () => { + const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [ + '--experimental-detect-module', + '--eval', + 'function fn() { await Promise.resolve(); } fn();', + ]); + + match(stderr, /SyntaxError: await is only valid in async function/); + strictEqual(stdout, ''); + strictEqual(code, 1); + strictEqual(signal, null); + }); + + it('permits declaration of CommonJS module variables', async () => { + const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [ + '--experimental-detect-module', + fixtures.path('es-modules/package-without-type/commonjs-wrapper-variables.js'), + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, 'exports require module __filename __dirname\n'); + strictEqual(code, 0); + strictEqual(signal, null); + }); + }); }); // Validate temporarily disabling `--abort-on-uncaught-exception` diff --git a/test/fixtures/es-modules/package-without-type/commonjs-wrapper-variables.js b/test/fixtures/es-modules/package-without-type/commonjs-wrapper-variables.js new file mode 100644 index 000000000000000..946bc690f27baa7 --- /dev/null +++ b/test/fixtures/es-modules/package-without-type/commonjs-wrapper-variables.js @@ -0,0 +1,6 @@ +const exports = "exports"; +const require = "require"; +const module = "module"; +const __filename = "__filename"; +const __dirname = "__dirname"; +console.log(exports, require, module, __filename, __dirname);