Skip to content

Commit

Permalink
module: do not set CJS variables for Worker eval
Browse files Browse the repository at this point in the history
PR-URL: #53050
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
aduh95 authored and targos committed Jun 1, 2024
1 parent 6022346 commit 4cdb05a
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 6 deletions.
2 changes: 1 addition & 1 deletion lib/internal/process/execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
18 changes: 13 additions & 5 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
28 changes: 28 additions & 0 deletions test/es-module/test-esm-detect-ambiguous.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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);
});
});

0 comments on commit 4cdb05a

Please sign in to comment.