From 4564644a938ccc680a6f0f1d2c95e14ae4befd77 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sat, 18 May 2024 23:00:34 +0200 Subject: [PATCH 1/5] module: do not set CJS variables for Worker eval --- lib/internal/process/execution.js | 2 +- src/node_contextify.cc | 16 +++++++++++----- test/es-module/test-esm-detect-ambiguous.mjs | 15 +++++++++++++++ 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/lib/internal/process/execution.js b/lib/internal/process/execution.js index 10f9e52244cac2..7834ce9aa74e8c 100644 --- a/lib/internal/process/execution.js +++ b/lib/internal/process/execution.js @@ -83,7 +83,7 @@ function evalScript(name, body, breakFirstLine, print, shouldLoadESM = false) { if (getOptionValue('--experimental-detect-module') && getOptionValue('--input-type') === '' && getOptionValue('--experimental-default-type') === '' && - containsModuleSyntax(body, name)) { + containsModuleSyntax(body, name, null, 'no CJS variables')) { return evalModuleEntryPoint(body, print); } diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 26aa4a206da37c..42c1175d29962d 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -1445,7 +1445,8 @@ static MaybeLocal CompileFunctionForCJSLoader(Environment* env, Local context, Local code, Local filename, - bool* cache_rejected) { + bool* cache_rejected, + bool is_cjs_scope) { Isolate* isolate = context->GetIsolate(); EscapableHandleScope scope(isolate); @@ -1498,7 +1499,10 @@ static MaybeLocal CompileFunctionForCJSLoader(Environment* env, options = ScriptCompiler::kConsumeCodeCache; } - std::vector> params = GetCJSParameters(env->isolate_data()); + std::vector> params; + if (is_cjs_scope) { + params = GetCJSParameters(env->isolate_data()); + } MaybeLocal maybe_fn = ScriptCompiler::CompileFunction( context, &source, @@ -1560,7 +1564,7 @@ static void CompileFunctionForCJSLoader( ShouldNotAbortOnUncaughtScope no_abort_scope(realm->env()); TryCatchScope try_catch(env); if (!CompileFunctionForCJSLoader( - env, context, code, filename, &cache_rejected) + env, context, code, filename, &cache_rejected, true) .ToLocal(&fn)) { CHECK(try_catch.HasCaught()); CHECK(!try_catch.HasTerminated()); @@ -1698,11 +1702,13 @@ static void ContainsModuleSyntax(const FunctionCallbackInfo& args) { CHECK(args[1]->IsString()); Local filename = args[1].As(); - // Argument 2: resource name (URL for ES module). + // Argument 3: resource name (URL for ES module). Local resource_name = filename; if (args[2]->IsString()) { resource_name = args[2].As(); } + // Argument 4: flag to indicate if CJS variables should not be in scope + bool cjs_var = !args[3]->IsString(); bool cache_rejected = false; Local message; @@ -1711,7 +1717,7 @@ static void ContainsModuleSyntax(const FunctionCallbackInfo& args) { TryCatchScope try_catch(env); ShouldNotAbortOnUncaughtScope no_abort_scope(env); if (CompileFunctionForCJSLoader( - env, context, code, filename, &cache_rejected) + env, context, code, filename, &cache_rejected, cjs_var) .ToLocal(&fn)) { args.GetReturnValue().Set(false); return; diff --git a/test/es-module/test-esm-detect-ambiguous.mjs b/test/es-module/test-esm-detect-ambiguous.mjs index 9d5f6f06a1c66b..86c15d3e4b436f 100644 --- a/test/es-module/test-esm-detect-ambiguous.mjs +++ b/test/es-module/test-esm-detect-ambiguous.mjs @@ -393,3 +393,18 @@ describe('Wrapping a `require` of an ES module while using `--abort-on-uncaught- strictEqual(signal, null); }); }); + +describe('when working with Worker threads', () => { + it('should work', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-detect-module', + '--eval', + 'new worker_threads.Worker("let __filename,__dirname,require,module,exports;this.a",{eval:true})', + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, ''); + strictEqual(code, 0); + strictEqual(signal, null); + }); +}); From e687acd34c56afe3a4dc6519156b0738e1818b4f Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sun, 19 May 2024 22:43:41 +0200 Subject: [PATCH 2/5] add test for `--eval` --- test/es-module/test-esm-detect-ambiguous.mjs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/es-module/test-esm-detect-ambiguous.mjs b/test/es-module/test-esm-detect-ambiguous.mjs index 86c15d3e4b436f..9ca813c3da7eb7 100644 --- a/test/es-module/test-esm-detect-ambiguous.mjs +++ b/test/es-module/test-esm-detect-ambiguous.mjs @@ -44,6 +44,19 @@ describe('--experimental-detect-module', { concurrency: !process.env.TEST_PARALL strictEqual(signal, null); }); + it('should not switch to module if code is parsable as script', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-detect-module', + '--print', + 'let __filename,__dirname,require,module,exports;this.a', + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, ''); + strictEqual(code, 0); + strictEqual(signal, null); + }); + it('should be overridden by --experimental-default-type', async () => { const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ '--experimental-detect-module', From 08c8705ecf29127e6dc110f7c18537d7ada569ce Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sun, 19 May 2024 22:44:24 +0200 Subject: [PATCH 3/5] Apply suggestions from code review Co-authored-by: Geoffrey Booth --- src/node_contextify.cc | 2 ++ test/es-module/test-esm-detect-ambiguous.mjs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 42c1175d29962d..bd85e1d8d5a41b 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -1708,6 +1708,8 @@ static void ContainsModuleSyntax(const FunctionCallbackInfo& args) { resource_name = args[2].As(); } // Argument 4: flag to indicate if CJS variables should not be in scope + // (they should be for normal CommonJS modules, but not for the + // CommonJS eval scope). bool cjs_var = !args[3]->IsString(); bool cache_rejected = false; diff --git a/test/es-module/test-esm-detect-ambiguous.mjs b/test/es-module/test-esm-detect-ambiguous.mjs index 9ca813c3da7eb7..e2b73f7436096e 100644 --- a/test/es-module/test-esm-detect-ambiguous.mjs +++ b/test/es-module/test-esm-detect-ambiguous.mjs @@ -408,7 +408,7 @@ describe('Wrapping a `require` of an ES module while using `--abort-on-uncaught- }); describe('when working with Worker threads', () => { - it('should work', async () => { + it('should evaluate a CommonJS worker as valid sloppy script where the CommonJS wrapper variables do not exist', async () => { const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ '--experimental-detect-module', '--eval', From 8ed8f2f446b33871db27019c3383bf4e44920f31 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sun, 19 May 2024 22:51:00 +0200 Subject: [PATCH 4/5] lint --- test/es-module/test-esm-detect-ambiguous.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/es-module/test-esm-detect-ambiguous.mjs b/test/es-module/test-esm-detect-ambiguous.mjs index e2b73f7436096e..40e44483ed21fd 100644 --- a/test/es-module/test-esm-detect-ambiguous.mjs +++ b/test/es-module/test-esm-detect-ambiguous.mjs @@ -408,7 +408,7 @@ describe('Wrapping a `require` of an ES module while using `--abort-on-uncaught- }); describe('when working with Worker threads', () => { - it('should evaluate a CommonJS worker as valid sloppy script where the CommonJS wrapper variables do not exist', async () => { + it('should support sloppy scripts that declare CJS "global-like" variables', async () => { const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ '--experimental-detect-module', '--eval', From 8ef5902d01553c3a2c689e3fc0489ff8d78c6b13 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Mon, 20 May 2024 13:33:54 +0200 Subject: [PATCH 5/5] Update test/es-module/test-esm-detect-ambiguous.mjs --- test/es-module/test-esm-detect-ambiguous.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/es-module/test-esm-detect-ambiguous.mjs b/test/es-module/test-esm-detect-ambiguous.mjs index 40e44483ed21fd..a58872e661c4f7 100644 --- a/test/es-module/test-esm-detect-ambiguous.mjs +++ b/test/es-module/test-esm-detect-ambiguous.mjs @@ -47,7 +47,7 @@ describe('--experimental-detect-module', { concurrency: !process.env.TEST_PARALL it('should not switch to module if code is parsable as script', async () => { const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ '--experimental-detect-module', - '--print', + '--eval', 'let __filename,__dirname,require,module,exports;this.a', ]);