Repository: nodejs/node
Pull request: module: do not set CJS variables for Worker eval

There are no local CJS variables in the Worker eval context – it's all global variables, just like in the `--eval` context.

<!--
Before submitting a pull request, please read:

- the CONTRIBUTING guide at https://github.com/nodejs/node/blob/HEAD/CONTRIBUTING.md
- the commit message formatting guidelines at
  https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines

For code changes:
1. Include tests for any bug fixes or new features.
2. Update documentation if relevant.
3. Ensure that `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes.

If you believe this PR should be highlighted in the Node.js CHANGELOG
please add the `notable-change` label.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
-->


Changed files:
filename: lib/internal/process/execution.js
patch: @@ -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);
   }
 

filename: src/node_contextify.cc
patch: @@ -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;

filename: test/es-module/test-esm-detect-ambiguous.mjs
patch: @@ -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);
+  });
+});