module: do not set CJS variables for Worker eval

PR-URL: #53050 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · May 25, 2024 · 2079a7a · 2079a7a
1 parent ccceae0
commit 2079a7a
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 6 deletions.
diff --git 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
@@ -1442,7 +1442,8 @@ static MaybeLocal<Function> CompileFunctionForCJSLoader(Environment* env,
                                                         Local<Context> context,
                                                         Local<String> code,
                                                         Local<String> filename,
-                                                        bool* cache_rejected) {
+                                                        bool* cache_rejected,
+                                                        bool is_cjs_scope) {
   Isolate* isolate = context->GetIsolate();
   EscapableHandleScope scope(isolate);
 
@@ -1494,7 +1495,10 @@ static MaybeLocal<Function> CompileFunctionForCJSLoader(Environment* env,
     options = ScriptCompiler::kConsumeCodeCache;
   }
 
-  std::vector<Local<String>> params = GetCJSParameters(env->isolate_data());
+  std::vector<Local<String>> params;
+  if (is_cjs_scope) {
+    params = GetCJSParameters(env->isolate_data());
+  }
   MaybeLocal<Function> maybe_fn = ScriptCompiler::CompileFunction(
       context,
       &source,
@@ -1556,7 +1560,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());
@@ -1694,11 +1698,15 @@ static void ContainsModuleSyntax(const FunctionCallbackInfo<Value>& args) {
   CHECK(args[1]->IsString());
   Local<String> filename = args[1].As<String>();
 
-  // Argument 2: resource name (URL for ES module).
+  // Argument 3: resource name (URL for ES module).
   Local<String> resource_name = filename;
   if (args[2]->IsString()) {
     resource_name = args[2].As<String>();
   }
+  // 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;
   Local<String> message;
@@ -1707,7 +1715,7 @@ static void ContainsModuleSyntax(const FunctionCallbackInfo<Value>& 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
@@ -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',
+        '--eval',
+        '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',
@@ -393,3 +406,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 support sloppy scripts that declare CJS "global-like" variables', 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);
+  });
+});